Schema fixes required for Aquilon support #177

jouvin · 2018-06-03T13:17:54Z

As explained in quattor/aquilon#90, current schema is not consistent with plenary templates generated by Aquilon (1.12.58). This PR reconciles them, preserving backward compatibility

This PR also improves the schema layout by factoring out of the base file hardware resources specific to Aquilon. And all the properties mandatory in the Aquilon DB and passed by the broker into the generated templates have been defined as mandatory.

stdweird · 2018-06-03T13:53:32Z

@jouvin i'm not going to review this, since i'm clueless wrt aquilon

jouvin · 2018-06-03T13:56:23Z

@stdweird but you are not clueless about the schema! Thus my choice to have you in the reviewers in case of something I missed...

jouvin · 2018-06-03T15:19:16Z

As explained in #142, /system/archetype/os type definition has been changed to a dict to avoid nasty regexp in configuration modules to find the OS name or version. This is not changed by this PR but ncm-cron configuration template was found buggy with respected to this schema change. quattor/configuration-modules-core#1282 fixes it and is also required for the Aquilon support. I did a quick search and it seems the only affected configuration module.

jrha

Looks fine to me. My only worry is that with the large number of optional entries it is impossible to rely upon any information being present, but I do understand that we are the mercy of the broker here.

jouvin · 2018-06-04T09:55:31Z

@jrha I also agree and probably we should make the schema a little bit more strict, in particular having all the values required by the broker (there are quite a lot) mandatory in the schema. We probably need to complete the discussion on things in the Aquilon DB not exported by the broker in the object template.

jrha · 2018-06-04T11:55:28Z

The problem of keeping the broker's view of the validation in step with the schema is a problem which still needs solving.

jouvin · 2018-06-04T11:59:18Z

Agreed! But I think this is more a problem of process than a technical issue. And the incentive to address it may be boosted by a larger adoption of Aquilon by the community!

jrha · 2018-06-04T12:04:00Z

There is a technical aspect too, any given version of the broker may be managing builds with any (or many) versions of the template library, so ideally the aquilon types should be provided by the broker itself.

jouvin · 2018-06-04T13:58:48Z

@jrha good point! In the meantime, I added a commit that defines as mandatory all the resources of aquilon.pan defined by the Aquilon (1.12.58) broker in the plenary templates.
This doesn't address the Aquilon-provided fields in hardware.pan (country, organisation...). As I don't think they are used by any non Aquilon site, should we factor them out of the main file in the same way this is done for system.pan so that we could define as mandatory everything provided by the broker?

jrha · 2018-06-04T14:01:41Z

Yes? It seems like a good idea.

jouvin · 2018-06-04T14:57:40Z

Done!

ned21 · 2018-06-04T21:21:16Z

quattor/types/aquilon/hardware.pan

@@ -0,0 +1,43 @@
+# This template extends the base schema with Aquilon-provied resources


provied => provided

ned21 · 2018-06-04T21:22:28Z

quattor/types/aquilon/hardware.pan

+    "room"       : string
+    "bunker"     ? string
+    "region"     ? string
+    "dns_search_domains" ? string[]


I don't remember seeing this in the structure_sysloc I sent you? This wouldn't really belong here I think? Nor location.

Sorry, I made a mistake trying to merge everything... In fact I wanted to delete them but incorrectly assume if was coming from you... Fixed!

ned21 · 2018-06-04T21:24:55Z

quattor/types/aquilon/system.pan

    @{ Details of operating system as defined by aquilon broker }
-    "os"            ? structure_archetype_os
+    "os"            : structure_archetype_os


Please revert this to "os" : string # e.g. "linux" since the broker has been generating this prior to the the change that introduced this dict.

See the complementary PR quattor/aquilon#94 for the broker...

ned21 · 2018-06-04T21:25:03Z

quattor/types/aquilon/system.pan

@@ -55,9 +55,11 @@ type structure_archetype_os = {
 };

 type structure_archetype = {
-    "name"          : string # e.g. "aquilon"
+    "name"          ? string # e.g. "aquilon"


should be mandatory to match ours.

I agree, it should... But as of Aquilon 1.12.60 it is not added to the hostdata by the broker... Should be fixed IMO... See quattor/aquilon#90

ned21 · 2018-06-04T21:25:14Z

quattor/types/aquilon/system.pan

-    "os"            ? structure_archetype_os
+    "os"            : structure_archetype_os
+    "os_lifecycle" : string
+    "model" ? string


Should be mandatory to match ours

Because of the broker proposed change, it is probably difficult to make it mandatory.

ned21 · 2018-06-04T21:25:33Z

quattor/types/aquilon/system.pan

    @{ Details of operating system as defined by aquilon broker }
-    "os"            ? structure_archetype_os
+    "os"            : structure_archetype_os
+    "os_lifecycle" : string


ned21 · 2018-06-04T21:26:34Z

quattor/types/aquilon/system.pan

@@ -112,7 +114,7 @@ type structure_personality = {
    "description"   ? string
    "class"         ? string with match(SELF, '(INFRASTRUCTURE|APPLICATION)')
    "users"         ? string[]
-    "systemgrn"     : string[]
+    "systemgrn"     ? string[]


Recent versions of the broker no longer generate this but I guess you need it for people on old broker versions?

No, I think we should require the last version of the broker for people wanting to use this version of the template library. I think we are only 3 sites running a broker and two of us probably up-to-date! Fixed.

ned21 · 2018-06-04T21:27:46Z

quattor/types/aquilon/system.pan

-    "stage"         ? string
+    "host_environment" : string with match(SELF, "^(dev|qa|uat|prod|infra|legacy)$")
+    "owner_eon_id"  : long
+    "stage"         : string


Making some of the mandatory could cause issues for users on an older version of the broker

See #177 (comment)

OK! In that case then we should also remove infra and legacy options because they are no longer valid.

jouvin · 2018-06-05T09:41:04Z

I think that at this stage this PR is complete and addressing all the @ned21 concerns, except the archetype/os. But for this issue, as it is not a changed made by this PR but a one year old change (17.7.0) missed by @ned21, I would prefer that it is not discussed as part of this PR (and doesn't block it) and that an issue is opened to rediscuss the change introduced by #142 if needed.

- Rack room made optional (optional in Aquilon rack object) - country added as an optional property to sysloc Contributes to quattor/aquilon#90

…support Contributes to quattor/aquilon#90

…e template - Every type definition related to Aquilon moved to quattor/types/aquilon directory - Some Aquilon-specific made mandatory to reflect their status in the Aquilon DB

jouvin · 2018-06-05T13:29:19Z

Rebased on #181... Please check! Not yet tested at LAL...

jouvin · 2018-06-05T13:53:59Z

Tested successfully at LAL with last broker version (1.12.60). The main broker problem not yet addressed is the missing archetype name in the hostdata that prevents marking this property required in the schema. @ned21 do you want the schema to be updated and a broker fix to be submitted or do we stay with the current situation in this PR (my preference) and will change it later once the broker does the appropriate thing...

jrha · 2018-06-05T14:07:36Z

@jouvin my feeling is that the schema should follow the broker.

jouvin · 2018-06-05T14:18:20Z

@jrha agreed. But @ned21 mentioned in one of his comment that he was expecting the archetype name to be required which is in fact not possible with the current broker. Thus my remark that we could decide to fix the broker to support the expected schema, even if I think it is not a good practice to create some sort of circular dependencies between issues and I'd prefer this to be addressed later, even if related to the original goal of this PR.

ned21

Two minor changes you could make to bring us back into alignment but happy to approve this if you don't want to make them.

On the subject of archetype/name: it's currently mandatory for us. However the broker doesn't specify it and it's a matter of philosophy as to how much we want the broker to dictate the schema since the tighter the coupling the more pain we'll feel with upgrades, etc. So I think it's fine to keep it as optional for now.

ned21 · 2018-06-06T17:38:44Z

quattor/types/aquilon/hardware.pan

+@documentation{
+    system location definition
+}
+type structure_sysloc = {


In our version all fields of this structure are optional. I think it might be best to stay in sync.

Fields changed to mandatory are those who are mandatory at the broker-level... See my answer to your comment

@jouvin changing these to mandatory has broken builds with our (admittedly ancient) broker 😞.

ned21 · 2018-06-06T17:40:35Z

quattor/types/aquilon/system.pan

-    "stage"         ? string
+    "host_environment" : string with match(SELF, "^(dev|qa|uat|prod|infra|legacy)$")
+    "owner_eon_id"  : long
+    "stage"         : string


OK! In that case then we should also remove infra and legacy options because they are no longer valid.

jouvin · 2018-06-06T19:08:18Z

On the broker-schema coupling, you are right that the tighter the more pain potentially... except if we find a way to remove these templates from the template library and have them distributed/generated by the broker.

The reason for the changes proposed here is to allow sites who want to rely on these properties to know which ones are always present and avoid too much testing in every template. So my preference would be to keep the proposed modifications.

jrha · 2018-07-12T11:18:58Z

@jouvin this can be merged if @ned21 is happy now

jouvin requested review from jrha, ned21 and stdweird June 3, 2018 13:17

jouvin added this to the 18.6 milestone Jun 3, 2018

jouvin mentioned this pull request Jun 3, 2018

1.12.58: machine template generated doesn't match profile_base schema quattor/aquilon#90

Closed

7 tasks

jrha approved these changes Jun 4, 2018

View reviewed changes

jouvin force-pushed the aquilon_schema branch 3 times, most recently from 3cfd61f to ead9da3 Compare June 4, 2018 14:55

ned21 requested changes Jun 4, 2018

View reviewed changes

jouvin force-pushed the aquilon_schema branch 4 times, most recently from e4efe90 to 76c0a85 Compare June 5, 2018 09:36

jouvin mentioned this pull request Jun 5, 2018

Revert /system/archetype/os changes #181

Merged

quattor schema (hardware): fixes for Aquilon support

e06f340

- Rack room made optional (optional in Aquilon rack object) - country added as an optional property to sysloc Contributes to quattor/aquilon#90

jouvin added 3 commits June 5, 2018 15:24

quattor schema (aquilon): add missing properties for Aquilon 1.12.58 …

f15a1d1

…support Contributes to quattor/aquilon#90

aquilon schema: define as mandatory all resources provided by the broker

d472a22

Schema refactoring to move Aquilon-specific HW resources to a separat…

f8d920a

…e template - Every type definition related to Aquilon moved to quattor/types/aquilon directory - Some Aquilon-specific made mandatory to reflect their status in the Aquilon DB

jouvin force-pushed the aquilon_schema branch from 76c0a85 to 71673a9 Compare June 5, 2018 13:28

jouvin force-pushed the aquilon_schema branch from 71673a9 to 2536ec5 Compare June 5, 2018 13:50

ned21 reviewed Jun 6, 2018

View reviewed changes

Aquilon schema: address @ned21 remarks on June 4 and 6

4be8df1

jouvin force-pushed the aquilon_schema branch from 2536ec5 to 4be8df1 Compare June 6, 2018 19:10

jrha modified the milestones: 18.6, 18.9 Jun 29, 2018

jouvin mentioned this pull request Jul 12, 2018

18.6.0-rc1 quattor/release#325

Closed

ned21 approved these changes Jul 12, 2018

View reviewed changes

ned21 merged commit db3f0c8 into quattor:master Jul 12, 2018

jouvin modified the milestones: 18.9, 18.6 Jul 12, 2018

jrha added the backwards incompatible label Jul 25, 2018

jrha mentioned this pull request Jul 25, 2018

18.6.0-rc3 quattor/release#326

Closed

jouvin mentioned this pull request Jul 25, 2018

Aquilon-related schema should be released with the broker quattor/aquilon#118

Open

		@@ -0,0 +1,43 @@
		# This template extends the base schema with Aquilon-provied resources

Schema fixes required for Aquilon support #177

Schema fixes required for Aquilon support #177

Conversation

jouvin commented Jun 3, 2018 • edited Loading

stdweird commented Jun 3, 2018

jouvin commented Jun 3, 2018

jouvin commented Jun 3, 2018 • edited Loading

jrha left a comment

Choose a reason for hiding this comment

jouvin commented Jun 4, 2018

jrha commented Jun 4, 2018

jouvin commented Jun 4, 2018

jrha commented Jun 4, 2018

jouvin commented Jun 4, 2018

jrha commented Jun 4, 2018

jouvin commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ned21 Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

jouvin commented Jun 5, 2018

jouvin commented Jun 5, 2018

jouvin commented Jun 5, 2018

jrha commented Jun 5, 2018

jouvin commented Jun 5, 2018

ned21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ned21 Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

jouvin commented Jun 6, 2018

jrha commented Jul 12, 2018

jouvin commented Jun 3, 2018 •

edited

Loading

jouvin commented Jun 3, 2018 •

edited

Loading

ned21 Jun 6, 2018 •

edited

Loading

ned21 Jun 6, 2018 •

edited

Loading