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.

Sign up for GitHub

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

Jump to bottom

Remove node-labeller #562

Merged
merged 3 commits into from
Jun 15, 2023
Merged

Remove node-labeller #562

merged 3 commits into from
Jun 15, 2023

Conversation

ksimon1
Copy link
Member

@ksimon1 ksimon1 commented Jun 2, 2023

What this PR does / why we need it:
feat: remove node labeller and bump api to v1beta2

Original credit goes to #492 and @lyarwood

Release note:

feat: remove node labeller and bump api to v1beta2

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jun 2, 2023
@ksimon1
Copy link
Member Author

ksimon1 commented Jun 2, 2023

/hold Need to change more things

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2023
@ksimon1 ksimon1 force-pushed the remove-nl branch 4 times, most recently from a1b8e36 to d08ec4d Compare June 6, 2023 10:09
0xFelix
0xFelix reviewed Jun 6, 2023
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

As this is right now it will break existing installations. Both API versions need to be served in parallel for existing installations to be able to upgrade. And you should ensure that the None conversion strategy is dropping unknown API fields. (see https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#field-pruning)

api/v1beta2/ssp_types.go Outdated
}

// +kubebuilder:object:root=true

Copy link
Member

Choose a reason for hiding this comment

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

nit: newline

go.mod Outdated
@@ -37,6 +37,8 @@ require (
sigs.k8s.io/yaml v1.3.0
)

require gopkg.in/yaml.v2 v2.4.0 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

Add this to the block below?

@0xFelix
Copy link
Member

0xFelix commented Jun 6, 2023

Can you also swap the order of commits, i.e. first introduce v1beta2, then change its schema?

@lyarwood
Copy link
Member

lyarwood commented Jun 9, 2023

Agree with @0xFelix that the commits need to be reordered here. We also need to retain and deprecate v1beta1 support IMHO to avoid immediately breaking any consumers of SSP such as HCO.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2023
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2023
@ksimon1
Copy link
Member Author

ksimon1 commented Jun 13, 2023

@akrejcir, @0xFelix, @lyarwood can you please review this?

0xFelix
0xFelix reviewed Jun 13, 2023
View reviewed changes
api/v1beta1/ssp_types.go
@@ -146,7 +146,7 @@ type SSPStatus struct {

// +kubebuilder:object:root=true
// +kubebuilder:subresource:status

// +kubebuilder:deprecatedversion:warning="ssp.kubevirt.io/v1beta1 ssp is deprecated"
Copy link
Member

@0xFelix 0xFelix Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is it necessary to add this explicitly? IIRC this is also the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we have to add it, otherwise generator will not add deprecate: true attribute.

Copy link
Member

Choose a reason for hiding this comment

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

ACK

config/crd/bases/ssp.kubevirt.io_ssps.yaml
@@ -3012,6 +3014,2038 @@ spec:
type: object
type: object
served: true
storage: false
Copy link
Member

Choose a reason for hiding this comment

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

Keep the storage version at v1beta1 until we have a proper migration/conversion strategy?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIU we don't need the conversion strategy, because we don't need to migrate any values

Copy link
Member

Choose a reason for hiding this comment

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

I'll have to test this but the None strategy just rewrites the APIVersion to the newer version, given the schema change I don't think that's going to work if the original object contains NodeLabeller?

Copy link
Member Author

Choose a reason for hiding this comment

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

Taken from: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#writing-reading-and-updating-versioned-customresourcedefinition-objects

if the default strategy value None is specified, the only modifications to the object are changing the apiVersion string and perhaps pruning unknown fields (depending on the configuration).

Which is exactly our use case, we just need to prune NL fields

Copy link
Member

Choose a reason for hiding this comment

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

Ah ha! I had forgotten the second part apologies, in that case None is fine.

akrejcir
akrejcir reviewed Jun 14, 2023
View reviewed changes
webhooks/ssp_webhook.go Outdated Show resolved Hide resolved
@akrejcir
Copy link
Collaborator

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2023
@lyarwood
Copy link
Member

@ksimon1 Would it be worthwhile adding NodeLabeller to the initial upgrade func test CR?

https://github.com/kubevirt/ssp-operator/blob/b8798993fc8932d152c07bb2009dabb16272150b/automation/e2e-upgrade-functests/ssp-cr-template.yaml

@lyarwood
Copy link
Member

@ksimon1 Would it be worthwhile adding NodeLabeller to the initial upgrade func test CR?

https://github.com/kubevirt/ssp-operator/blob/b8798993fc8932d152c07bb2009dabb16272150b/automation/e2e-upgrade-functests/ssp-cr-template.yaml

AFAICT we would also need to rework the test to upgrade/deploy the operator itself from the local source tree for such a test to be valid:

https://github.com/kubevirt/ssp-operator/blob/b8798993fc8932d152c07bb2009dabb16272150b/automation/e2e-upgrade-functests/run.sh

@lyarwood
Copy link
Member

@ksimon1 Would it be worthwhile adding NodeLabeller to the initial upgrade func test CR?
https://github.com/kubevirt/ssp-operator/blob/b8798993fc8932d152c07bb2009dabb16272150b/automation/e2e-upgrade-functests/ssp-cr-template.yaml

AFAICT we would also need to rework the test to upgrade/deploy the operator itself from the local source tree for such a test to be valid:

https://github.com/kubevirt/ssp-operator/blob/b8798993fc8932d152c07bb2009dabb16272150b/automation/e2e-upgrade-functests/run.sh

Apologies I missed the deploy at the end of the script so we are upgrading correctly. I think my suggestion of adding NodeLabeller to the initial CR is still valid and should ensure it doesn't cause any issues in v1beta2 once it's dropped.

@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2023
@lyarwood
Copy link
Member

/lgtm

Thanks!

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2023
@ksimon1
chore: introduce v1beta2 api
3ffa456 
this api removes node labeller api fields which are no longer supported.
NL was moved from SSP operator to kubevirt with:
kubevirt/kubevirt#4916.

Signed-off-by: Karel Šimon <ksimon@redhat.com>
@ksimon1
chore: update api version in code
eb60b15 
this commit updates paths of new api version in code
and removes deprecated node-labeller api fields

Signed-off-by: Karel Šimon <ksimon@redhat.com>
@ksimon1
chore: regenerate base manifests
237f966 
This commit regenerates manifests with command `make container-build`.
Inserted file with conversion strategy, which add
spec.conversion.strategy: none field.

Signed-off-by: Karel Šimon <ksimon@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2023
@akrejcir
Copy link
Collaborator

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2023
@ksimon1
Copy link
Member Author

ksimon1 commented Jun 14, 2023

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ksimon1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jun 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.8% 1.8% Duplication

@lyarwood
Copy link
Member

lyarwood commented Jun 14, 2023 •
edited
Loading

@ksimon1 Any reason to hold this anymore?

/approve

Also how on earth are you able to /approve your own PR?!

@ksimon1
Copy link
Member Author

ksimon1 commented Jun 15, 2023

/hold cancel
If you have approve permissions, you can approve any PR :)

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2023
@kubevirt-bot kubevirt-bot merged commit b5e8ed2 into kubevirt:master Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xFelix 0xFelix 0xFelix left review comments

@akrejcir akrejcir akrejcir left review comments

@lyarwood lyarwood Awaiting requested review from lyarwood

Assignees

@lyarwood lyarwood

@akrejcir akrejcir

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ksimon1 @0xFelix @lyarwood @akrejcir @kubevirt-bot