-
Notifications
You must be signed in to change notification settings - Fork 20
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
Change site and machine namespace to use continent name #110
base: upstream
Are you sure you want to change the base?
Conversation
I don't think hosts need to be rebuilt do they? It just needs a flush of the host plenaries and a co-ordindated update to all template domains. Anyway this looks a sensible approach to me, but I think the unit tests will need updating too. |
You are right, a flush is enough but a recompile also if you want to assess that everything is ok, isn't it? |
@@ -40,7 +40,7 @@ class PlenaryMachineInfo(StructurePlenary): | |||
@classmethod | |||
def template_name(cls, dbmachine): | |||
loc = dbmachine.location | |||
return "%s/%s/%s/%s/%s" % (cls.prefix, loc.hub.fullname.lower(), | |||
return "%s/%s/%s/%s/%s" % (cls.prefix, loc.continent.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have discussed this with Gabor just now: he suggest to take this even further and get rid of location attributes from the template path: and leave only (cls.prefix, dbmachine.label). Would this work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem for me... but will mean a directory with thousands or ten of thousands entries... Is it really what you want? I agree that the current hierarchy is probably too much but keeping the continent+building or continent+city or even just the city may be not so bad....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about creating a structure using the first letters of the dbmachine.label ?
For instance:
return "{0}/{1:.1}/{1:.2}/{1}".format(cls.prefix, dbmachine.label)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting suggestion, easy to implement... Not sure how diverse the machine names are, in particular their first letter. Ours tend to be something like rackXXYY
where XX
is the rack number and YY
the slot number in the rack. Will defeat the directory hierarchy created from the first letters in our case... but probably not a major issue for us if everybody agrees it is the way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our machine names come from another system and start with one of: system
, scarf
, vm
or test
; so would have a similar problem, OTOH we only have 23000 hosts managed by the brokers, so not a huge problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we really need to break it up, how about using a modulus of the hardware_entity_id
?
return "%s/%02x/%s" % (cls.prefix, dbmachine.id % 256, dbmachine.label)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either that, or if you want something more ordered, how about groups of no more than 1000?
return "%s/%02d/%s" % (cls.prefix, dbmachine.id / 1000, dbmachine.label)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrha I think it would be good to have it easy to deduce from the machine information. dbmachine.id / 1000
is probably not the most convenient, even if it is predictible.
@@ -30,7 +30,7 @@ class PlenaryCity(Plenary): | |||
|
|||
@classmethod | |||
def template_name(cls, dbcity): | |||
return "%s/%s/%s/config" % (cls.prefix, dbcity.hub.fullname.lower(), | |||
return "%s/%s/%s/config" % (cls.prefix, dbcity.continent.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cannot yet merge this cause we need to fix a 'bug' in city templates for timezones. but in general we agree with the change - just let us prepare for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem.
@@ -629,18 +629,20 @@ def transaction(self, verbose=False): | |||
|
|||
|
|||
def add_location_info(lines, dblocation, prefix=""): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general we agree - it is a good fix - but we have to make huge template change change to allow this to go out cause we have some sysloc template code that would start failing if we just roll this out... I suggest to create a separate PR for this if we want to release the other changes sooner :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look it if makes sense but will probably not do anything before end of August anyway... What's your timeframe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urbonegi Looking in more details, I don't understand this comment. You mean that adding sysloc/region
will make your existing code failing? Else the modification doesn't change anything to the current template contents (except a minor sysloc property reordering, with rack name before the rack info).
c864647
to
584c978
Compare
- Used to be hub fullname previously which is not exported to the object plenary, making impossible to locate the site template (not included by default in the object plenary) - Hosts must be rebuilt to use the new namespace Fixes quattor#104 Change-Id: I71e59ffa750a264b88ecf35ae3e6ebecd73f98e2
- Contributes to quattor#104 Change-Id: I061147728c376e722b1ffd67665921416e1291d8
The hub fullname was used previously but as it was not exported to the object plenary, it was impossible to locate the site template (not included by default in the object plenary)
This PR also adds the hub information in the object plenary under
/hardware/sysloc/region
.Fixes #104
Change-Id: I71e59ffa750a264b88ecf35ae3e6ebecd73f98e2