Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.12.58: city timezone is not passed to the object template #91

Open
jouvin opened this issue May 28, 2018 · 22 comments
Open

1.12.58: city timezone is not passed to the object template #91

jouvin opened this issue May 28, 2018 · 22 comments
Assignees

Comments

@jouvin
Copy link
Contributor

jouvin commented May 28, 2018

The broker doesn't output to the object profile the city timezone information, making impossible to configure it at installation time (or later with Quattor).

A proper fix of this issue requires #104 to be fixed first.

@ned21
Copy link
Contributor

ned21 commented May 28, 2018

The broker does not have a "timezone" field against the host object so it cannot appear in the plenary. The city object has a timezone field so you can use business logic in templates to load the right localised config, or UTC, or whatever timezone makes sense for you.

For some reason we load this information as part of our OS template but I think it should be part of archetype/base really. Can you please include it in your sample base.pan?

include { if_exists("site/" +
        value("/hardware/sysloc/region") + "/" +
        value("/hardware/sysloc/city") + "/" +
        value("/hardware/sysloc/building") + "/" +
        "config") };
include { if_exists("site/" +
        value("/hardware/sysloc/region") + "/" +
        value("/hardware/sysloc/city") + "/" +
        "config") };
include { if_exists("site/" +
        value("/hardware/sysloc/region") + "/" +
        "config") };

@ned21 ned21 closed this as completed May 28, 2018
@jouvin jouvin reopened this May 28, 2018
@jouvin
Copy link
Contributor Author

jouvin commented May 28, 2018

@ned21 I don't totally agree. There is a timezone information in Aquilon attached to the city and this one is not available in the object template. It would be much better to have this information in the template so that we can use it, as it is a required information when adding a city rather than having to use a set of template do define (again) the timezone at the city, region or country level... I think it is a better default behaviour that doesn't prevent a site from doing your approach if there is a reason for it. In general, IMO, an Aquilon user (at least me!) doesn't expect to redeclare in templates things that were configured in Aquilon DB.

@ned21
Copy link
Contributor

ned21 commented May 28, 2018

I think this is yet another example where failing to open source our templates has caused significant problems and misunderstandings. The plenary templates are just a copy of what is held in the table associate with that object. So if there's no field in the table, it cannot appear in the plenary of the same name. We could do the join in the broker code but the principle (which predates me) is to make the DB as "dumb" as possible and do business logic in templates because the broker does not want to make assumptions about your business logic. I think that's something that originally came from Quattor. :)

So yes this logic should be exposed and made easy for the Quattor/Aquilon user, but that doesn't mean it should be done with PAN not python.

@jrha
Copy link
Member

jrha commented May 29, 2018

But in this case there is a field in the table for the timezone.

@jrha
Copy link
Member

jrha commented May 29, 2018

@ned21 I think that the counterpoint is that it is reasonable to expect that any field that exists in an object's table should also appear in the plenary template for that object, which is what I think @jouvin is getting at. If the DB data is not exposed in the plenaries, it cannot be used for business logic.

@jouvin
Copy link
Contributor Author

jouvin commented May 29, 2018

@jrha you summarized well my point 😄

@jrha
Copy link
Member

jrha commented May 29, 2018

Looking into it a bit more @ned21 is also correct, there is a plenary for the city and it does include the time-zone, it just isn't included anywhere, the code he provided would accomplish that.

e.g.

cat /var/quattor/cfg/plenary/site/gb/harwell/config.tpl
template site/gb/harwell/config;

variable TIMEZONE = "UTC";

@jouvin
Copy link
Contributor Author

jouvin commented May 29, 2018

Ah... I misunderstood @ned21 previous comment about these templates I thought we should create them... I guess we can close the issue then

@ned21
Copy link
Contributor

ned21 commented May 29, 2018

Sorry I wasn't clearer. I meant to include the "aq cat --city ln" output.

@ned21 ned21 closed this as completed May 29, 2018
@jouvin jouvin reopened this May 30, 2018
@jouvin
Copy link
Contributor Author

jouvin commented May 30, 2018

I'm reopening this issue because I didn't fully understand how the city template under plenary site directory (defining the timezone) is supposed to be included... Under site directory, cities are grouped by hub if I understood properly. But this hub info doesn't seem to be passed to the object profile (and the other generated templates) by the broker... Did I miss something?

@ned21
Copy link
Contributor

ned21 commented May 30, 2018

oh, this rings a bell - when we set up a new Aq instance internally last year we came across this problem where the location had changed.

@ajf8 - what was the fix?

Is it definitely the hub or the campus? We have hubs and campuses with the same names so I can't tell. This is the full list of location data populated into the plenary:

"sysloc/continent" = "eu";
"sysloc/country" = "gb";
"sysloc/city" = "ln";
"sysloc/campus" = "ln";
"sysloc/building" = "cw";
"location" = "cw.ln.eu";

If it's the campus then you should be able to do:

include { if_exists("site/" +
        value("/hardware/sysloc/campus") + "/" +
        value("/hardware/sysloc/city") + "/" +
        "config") };

@jouvin
Copy link
Contributor Author

jouvin commented May 30, 2018

In fact, I think this the hub as in one of my example I don't have campus defined and in the other one I have it but with a name different from the hub and I can't see it in the hostdata. (hardware sysloc)..

@ned21
Copy link
Contributor

ned21 commented May 30, 2018

Can you share the output of aq cat --machine $machine?

@jouvin
Copy link
Contributor Author

jouvin commented May 30, 2018

For example, I have a machine testhw. Its Aquilon DB definition is:

(aquilon) [aquilon@aquilon ~]$ aq show machine --machine testhw
Machine: testhw
  Primary Name: testsrv.dailyplanet.com [192.168.1.3]
  Building: hq
  Campus: bigcampus
  City: metropolis
  Continent: america
  Country: us
  Hub: pacific
  Organization: daily_planet
  Rack: hq4
    Row: r
    Column: c
  Room: supersecret
...

But the associated plenary template doesn't contain the hub and campus information:

 aq cat --mach testhw
structure template machine/pacific/hq/hq4/testhw;

"sysloc/continent" = "america";
"sysloc/country" = "us";
"sysloc/city" = "metropolis";
"sysloc/building" = "hq";
"rack/name" = "hq4";
"rack/row" = "r";
"rack/column" = "c";
"sysloc/room" = "supersecret";
"location" = "hq.metropolis.america";

"nodename" = "testhw";
include { "hardware/machine/luthorindustries/thegreatserver" };
....

And as you can see below the city configuration is under site/$hub:

(aquilon) [aquilon@aquilon ~]$ ls /pdisk/quattor/cfg/plenary/site/
pacific  upsaclay
(aquilon) [aquilon@aquilon ~]$ ls /pdisk/quattor/cfg/plenary/site/pacific/metropolis/
config.pan

IMO, something is inconsistent because there is no way to select the city template from the information in the plenary object. And as said before, some information available in Aquilon DB is not passed to the plenary object.
BTW, I have the feeling that only campus are optional... Hubs are close to the top of the geographical hierarchy and is not optional. A hub is potentially a group of continent/country(/campus)/city.

@jouvin
Copy link
Contributor Author

jouvin commented May 30, 2018

Thinking more at the site directory structure, I think there is something wrong in the choice $hub/city as the hub is a superset of continent and countries. Take for example Paris which can be at least Paris/France and Paris Texas. Imagine that for some reason they are part of the same hub made of america/us and europe/france, then there is a clash. It looks to me that at least country is needed to make the pan namespace unique for each city (I don't have in mind two countries with the same name in 2 different continents! So probably continent is not needed).

@ajf8
Copy link

ajf8 commented May 31, 2018

These lines of code may explain the confusion, which I had to dig into myself when we setup a new broker recently. It does expose the timezone into a plenary, but the path for the plenary doesn't really make sense and should probably be changed.

https://github.com/quattor/aquilon/blob/upstream/lib/aquilon/worker/templates/city.py#L33

Notice it uses the fullname attribute (lowercased) of the hub. If you don't specify a fullname with add_hub, it will default to the name of the hub.

The way this works for us (see the pan code @ned21 showed), is that we have the fullname of the hub set to match /hardware/sysloc/region. For example, the fullname for our "ny" hub is "Americas". So the ny city plenary is written to site/americas/ny/config.pan.

Yes, using the fullname of the hub is weird. And there's no update_hub command, so I had to fix this with a SQL statement.

@ned21 ned21 added bug and removed bug labels May 31, 2018
@ned21
Copy link
Contributor

ned21 commented May 31, 2018

So I think we've identified two bugs here:

  1. using fullname instead of the code for hub
  2. hub is not exposed in the machine plenary so it's not possible to load the city plenary containing the TIMEZONE

We haven't fixed 1 because it would break our existing install, and presumably RAL too. (2) could be fixed more easily but first requires an update to structure_sysloc which currently looks this this:

type structure_sysloc = {
  "room"       ? string
  "bunker"     ? string
  "building"   ? string
  "city"       ? string
  "country"    ? string
  "region"     ? string
  "continent"  ? string
  "campus"     ? string
};

And unless (1) is fixed, it's not possible to actually use it. Or we have to do something odd and put the fullname in the sysloc structure not the code. (And full names are free text so may not be valid PAN=>can of worms).

tbh I am not sure using hub in the directory path makes sense at all since it's a logical construct. The plenary ought to be under country or some other geographical hierarchy. I found the original commit from @njwilliams in 2010 and from reading the commit message I am guessing he used the lowercase case fullname as a hack to get backwards compatibility with the directory schema that was already in use in template-king. (Aside: detailed commit messages FTW!)

I don't think changing this for any site is impossible but it does require a co-ordinated template-king update which we have previously found difficult as we have multiple active domains and tens of template developers who must update their sandboxes before hosts can compile again. I therefore would not like to assume it is easy for any existing Aquilon site to make this sort of change.

Having said that, it would be nice to have a config setting that fixed this for new sites. However before we do that we need to decide what "fixed" looks like. City names are globally unique (the name is really an identifier, fullname is free text). I suspect most sites do not have too many either so the hierarchy should not need to be deep. But it probably should not be flat either. Continent most closely matches what we use today and would keep the hierarchy the same depth. It also appears in the machine plenary.

@jouvin
Copy link
Contributor Author

jouvin commented Jun 1, 2018

@ned21 thanks for your detailed analysis! Thinking at this yesterday, I had an idea that may help to solve this problem without breaking the backward compatibility! I find the template machinery invented to pass the timezone to the plenary template overwhelmingly complicated... At the end this is just to define one variable
.
My proposal is to deprecated the use of the template (but continue to produce it for backward compatibility, with the same contents) and to pass this information in the host data (extending the schema to add a timezone property`). This way it would be very simple to get the timezone without any dependency on the city, hub... For the broker, it is easy to find the timezone for a given host, based on its location.

The proposed approach would also allow to address your use case where you want to override the city timezone for a host. Rather than hacking/hooking this into the template, we could had an optional timezone to the host object and have the broker using it if it is defined (rather than the mandatory city timezone) for defining the timezone in the host data. This will be much more trackable, IMO.

Last advantage is that this mechanism is backward compatible as it it up to a site to decide to use the new mechanim (that will potentially produce new values) or keep the old one whose value will remain unchanged, even if we had the optional timezone for the host object.

@jrha
Copy link
Member

jrha commented Jun 1, 2018

Fixing (1) won't break RAL, as we don't currently make use of it in a meaningful way.

@ned21
Copy link
Contributor

ned21 commented Jun 1, 2018

It's a good idea but I think having a timezone field in the host table would be contrary to Normal Form, and/or require the user to type --timezone when adding a host which would still be a breaking change to any automations that call add_host.

We only use the site/ location files for timezone because we pass all the other geographic info via services but the mechanism exists to allow people to load per-site content in a flexible manner. e.g. if you want to specify your NTP_SERVERS templates via templates instead of the broker then that would be the way to do it.

@jouvin
Copy link
Contributor Author

jouvin commented Jun 1, 2018

@ned21 I perfectly understand the reason for having location-related templates and I am not suggesting to remove this possibility (in fact there is no way to remove it as the use case you describe means they are site templates rather than plenary templates). I was suggesting to add another mechanism to configure the timezone that could be used as an alternative approach when it is appropriate and will allow to maintain the backward compatibility for existing sites.

I don't suggest to break the Normal Form! My suggestion is to have this new field present in every host with the possibility of being empty by default which AFAIK is perfectly valid. When this happens, the broker, when generating the plenary templates, will use the city timezone information for the host location to fill whatever pan configuration path we agree on (or variable TIMEZONE but I understood that you have a preference for configuration paths). One of your template examples, https://github.com/ned21/template-library-examples/blob/archetype-example/aquilon/example/archetype/final.pan, was mentioning /system/timezone that I like (but is not currently part of the schema, easy to add).

My proposal is to maintain the current contents of the city-related templates for a transition period after which all sites should have migrated them to their site templates as they would no longer be generated by the broker (but remain usable as long as they exist).

@jouvin jouvin changed the title 1.12.58: city timezone is not passed to the object template 1.12.58: location related templates cannot be used Jul 5, 2018
@jouvin jouvin changed the title 1.12.58: location related templates cannot be used 1.12.58: city timezone is not passed to the object template Jul 6, 2018
@jouvin
Copy link
Contributor Author

jouvin commented Jul 6, 2018

As this issue is now 2 different issues, the location templates namespace and the timezone information in plenary templates, I created a new issue to track the namespace issue and copied @ned21 detailed analysis of the problem from this issue.

I suggest to keep this issue only for the timezone discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants