-
Notifications
You must be signed in to change notification settings - Fork 19
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
networking: use the main registration websocket channel for network data #820
Conversation
ae1c7d9
to
3c1b700
Compare
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.
Appending the serialized NetworkConfig to the MachineRegistration may create collisions in the future. I feel this is a bit too uncharted territory. The new data should have a proper schema at very least.
log.Errorf("error marshalling network config: %v", err) | ||
return err | ||
} | ||
data = append(data, netData...) |
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.
This may be problematic.
The registration (what's given in the data
) already contains the networkTemplate.
Some keys are colliding with the network config, so not sure how this is supposed to work.
We do need a different API in here, this should be an object, not appended. Most likely we need a wrapper struct that contains both the MachineRegistration (like data), but additionally it contains the NetworkConfig in some nested field as well.
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.
This may be problematic. The registration (what's given in the
data
) already contains the networkTemplate. Some keys are colliding with the network config, so not sure how this is supposed to work.
well, actually no, they do not collide, since yaml data is structured: data
here holds elemental, network and cloud-config while netData
adds ipAddresses and config sections.
We do need a different API in here, this should be an object, not appended. Most likely we need a wrapper struct that contains both the MachineRegistration (like data), but additionally it contains the NetworkConfig in some nested field as well.
We have multiple problem we inherited with the "original" protocol, in particular here sharing the data structure used for defining the elemental config in the CRDs to also send installation configuration.
We also have the end of connection bound to sending back the full installation config (sob).
The idea here was to avoid adding another "get config and close" API in the protocol and have to maintain two of them... we already did in the past, was not great.
All the data is serialized in bytes, appending two configs is not at all a programming pattern, not something that would in general recommend, but for this case it will allow to keep the current API, piggyback network information if available, and would work with old register client seamlessly thanks at how yaml (and json) marshaling works.
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.
This may be problematic. The registration (what's given in the
data
) already contains the networkTemplate. Some keys are colliding with the network config, so not sure how this is supposed to work.well, actually no, they do not collide, since yaml data is structured:
data
here holds elemental, network and cloud-config whilenetData
adds ipAddresses and config sections.We do need a different API in here, this should be an object, not appended. Most likely we need a wrapper struct that contains both the MachineRegistration (like data), but additionally it contains the NetworkConfig in some nested field as well.
We have multiple problem we inherited with the "original" protocol, in particular here sharing the data structure used for defining the elemental config in the CRDs to also send installation configuration. We also have the end of connection bound to sending back the full installation config (sob).
The idea here was to avoid adding another "get config and close" API in the protocol and have to maintain two of them... we already did in the past, was not great.
All the data is serialized in bytes, appending two configs is not at all a programming pattern, not something that would in general recommend, but for this case it will allow to keep the current API, piggyback network information if available, and would work with old register client seamlessly thanks at how yaml (and json) marshaling works.
Agreed, not having to reopen the websocket is indeed nice.
For the data I see it now it's how we are already doing it, nevermind. It is not going to collide indeed.
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.
LGTM
use the same websocket connection to exchange all the data Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
3c1b700
to
7c48972
Compare
Now the network data is piggybacked to the elemental data during registration.
This way the registering client doesn't need to open a separate connection after the registration is done.