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

machine registration connectivity rework - part III #143

Merged
merged 6 commits into from
Sep 1, 2022
Merged

Conversation

fgiudici
Copy link
Member

@fgiudici fgiudici commented Aug 25, 2022

This PR changes the communication protocol between the operator and the elemental-register client in order to take full advantage of the websocket connection: SMBIOS data and labels are now passed via the websocket channel.
The protocol is extensible and could be easily extended to pass arbitrary data or request different kind of services to the operator.
Protocol discussion: #152

The change breaks backward compatibility with older operators and elemental-register clients.

Fixes #5

@github-actions github-actions bot added area/build build related changes area/operator operator related changes area/register register related changes area/tests test related changes labels Aug 25, 2022
@fgiudici fgiudici added kind/enhancement New feature or request Blocked and removed area/operator operator related changes area/register register related changes area/tests test related changes area/build build related changes labels Aug 25, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #143 (fdc9a1b) into main (f0bd8f4) will decrease coverage by 8.89%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   33.77%   24.88%   -8.90%     
==========================================
  Files           5        5              
  Lines         379      422      +43     
==========================================
- Hits          128      105      -23     
- Misses        246      313      +67     
+ Partials        5        4       -1     
Impacted Files Coverage Δ
pkg/server/register.go 9.95% <0.00%> (-14.03%) ⬇️
pkg/server/server.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fgiudici fgiudici requested review from a team and removed request for a team August 26, 2022 07:28
@github-actions github-actions bot added area/build build related changes area/operator operator related changes area/register register related changes area/tests test related changes labels Aug 26, 2022
}
err = SendJSONData(conn, MsgSmbios, data)
if err != nil {
return fmt.Errorf("sending SMBIOS data: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would add debug in here with the data that failed to send in case we find some strange data or broken data (hit: litter.sdump will nicely print it instead of getting a terrible string)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, nice advice the linter package 😄
done! also for the labels... not needed as much as the SMBIOS data (labels are much more under control) but why not? 😁

}

switch msgType {
case register.MsgSmbios:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a-we-some!

Copy link
Contributor

@Itxaka Itxaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

freaking awesome!

@Itxaka
Copy link
Contributor

Itxaka commented Aug 31, 2022

Unfortunately, keeping both backwards and forwards compat seems kind of difficult IMO, I would just break and cut 0.5.0 directly now that we can and this seems more future proof in case we want to change something in the workflow like adding more stuff.

Really nice job!

Copy link
Contributor

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice

@davidcassany
Copy link
Contributor

Unfortunately, keeping both backwards and forwards compat seems kind of difficult IMO, I would just break and cut 0.5.0 directly now that we can and this seems more future proof in case we want to change something in the workflow like adding more stuff.

Even if it would be reasonable to keep both I don't think it is a good idea. If breaking changes are convenient in the elemental-operator to make the node-server communication robust now it is the time. Later on it will be harder to make breaking changes, now it is a good time to get rid of the most prominent tech debt.

@github-actions github-actions bot removed area/register register related changes area/build build related changes labels Aug 31, 2022
@fgiudici
Copy link
Member Author

rebased on main branch. Also added unit-test coverage for the websocket rework.

@fgiudici fgiudici removed the Blocked label Sep 1, 2022
We already had a deadline on the operator side: add to the
registering client to.
Share websocket configuration options as we will soon need it
for establishing a common protocol.

Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
We are going to rework the communication protocol, making full use
of the bidirectional websocket we set up. Here we introduce some
helper functions that will make the communication easier.

Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
This commit changes the communication protocol between the operator and
the elemental-register client in order to take full advantage of the websocket
connection: SMBIOS data and labels are now passed via the websocket channel.
The protocol is now extensible and could be easily extended to pass arbitrary
data or request different kind of services to the operator.

This commit breaks backwards compatibility with older operators and
elemental-register clients.

Fixes #5

Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
remove old code passing data via HTTP labels

Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
@Itxaka
Copy link
Contributor

Itxaka commented Sep 1, 2022

meeeeeerge!

@fgiudici fgiudici merged commit 03628b7 into main Sep 1, 2022
@fgiudici fgiudici deleted the websocket_reg03 branch September 1, 2022 14:26
@davidcassany
Copy link
Contributor

Can't wait for a v0.5.0 🤣

@fgiudici
Copy link
Member Author

fgiudici commented Sep 1, 2022

🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operator operator related changes area/tests test related changes kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chunked upload of machineInventory smbios data
4 participants