Add bypassValidation flag to disable strict xname-checking optionally#72
Add bypassValidation flag to disable strict xname-checking optionally#72bmcdonald3 wants to merge 3 commits into
Conversation
Signed-off-by: Ben McDonald <ben.mcdonald@hpe.com>
|
One thing I'll quickly mention while this is still a draft is I think we've recently decided that we generally prefer affirmative flags that we can set to false (e.g. |
|
Is this ready for review? |
|
I don't understand this change or why it is needed. Can you add unit tests that demonstrate the behavior with the flag both true and false? |
|
@davidallendj I haven't had a chance to test thoroughly, but I think it's ready to go -- working on some FRUTracking stuff at the moment, but can give this a real test tomorrow after some meetings. @alexlovelltroy This change is because, today, SMD requires all components that are added to the database adhere to xname conventions, which was a requirement in CSM. This was required because other services and SMD discovery (which we don't use in OpenCHAMI) depended on xnames. Since we don't have a strict xname requirement, removing the hard requirement in code allows more flexibility in what you can store in SMD (for example, emulators that don't have a real xname). Once this is ready to go, I will add things like a description and some testing, I just am not currently prioritizing this task. |
|
xnames appear to be core to CSM. What about code in other services that assumes all IDs will be parseable as xnames? Looks to me like this change would invalidate assumptions in other parts of the code. Have you checked swagger files to see what guarantees are advertised about xnames? |
|
That's a good point, I checked out the swagger file and also looked within OpenCHAMI at what other services are doing with the xnames. You are right, within CSM, xnames are widely used in things like loftsman, CSI, istio, etc., so if we are hoping to pull in more CSM services down the line, we probably want to keep xnames as a requirement (though, I thought they weren't today, but I may have missed something there). For the, API Contract the Swagger documentation consistently refers to component IDs as xnames, establishing a strong implicit contract, though it is not formally enforced with a pattern (like it is for MAC addresses and UUID). Searching through the OpenCHAMI repos, I don't believe any services today parse the xnames of It is a good point that, as SMD heavily references xnames (even if it's mainly in things we don't use in OpenCHAMI or things that could be addressed in this PR), it is a big conceptual change to just get rid of that. I think things would probably get more complicated, rather than easier in the future if we get rid of that xname requirement, so I am fine just closing this and calling it good there. @davidallendj are you alright with closing this considering our discussion last week? On a related note, this investigation highlights a separate issue regarding identifier generation. As noted in Eric's PR (OpenCHAMI/magellan#109), Magellan currently generates identifiers that are only superficially xnames. They pass SMD's validation but are not canonical and may not be fully compatible with tooling that expects strict xname formatting. This seems to be what you are hinting at and I believe it is a distinct problem from validation, but I think they are both practically bypassing the same thing (i.e., what we generate today "doesn't work" in the same way that bypassing "doesn't work", since neither of them correspond to actual hardware, but Magellan's approach gets past the initial checks, which is arguably a better solution than this one). |
|
I think we can close this PR. I think we agreed to continue using the old xname generation in Magellan to bypass the check until we figure out what we want to do collectively moving forward with IDs and naming mentioned in OpenCHAMI/roadmap#10. |
No description provided.