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 deprecated CiliumEndpoint fields #10509

Merged
merged 2 commits into from Mar 9, 2020
Merged

Remove deprecated CiliumEndpoint fields #10509

merged 2 commits into from Mar 9, 2020

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Mar 7, 2020 •
edited

This change is Reviewable

@tgraf
api: Remove deprecated CiliumEndpoint resource fields
57650ab 
These fields have been deprecated in Cilium 1.4 and were scheduled for removal
in 1.5. Remove them for 1.8.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
api: Remove unused StateCreating
9281012 
This state no longer exists and has not been in use for a while, remove it.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Mar 7, 2020
@tgraf tgraf requested review from a team as code owners March 7, 2020 20:49
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Mar 7, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 45.613% when pulling 9281012 on pr/tgraf/remove-deprecated-endpoint-fields into 0d7d76c on master.

@tgraf
Copy link
Member Author

tgraf commented Mar 7, 2020 •
edited

test-me-please

Suite-k8s-1.11.K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with direct routing Tests NodePort failed

@tgraf
Copy link
Member Author

tgraf commented Mar 8, 2020

test-me-please

aanm
aanm requested changes Mar 9, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Because the endpoint status structure is being modified we need to bump

cilium/pkg/k8s/apis/cilium.io/v2/register.go

Line 45 in 9779b09

CustomResourceDefinitionSchemaVersion = "1.16"

Also, by bumping that version, we will brake downgrade path as that structure will no longer exist if the user wants to downgrade from 1.8 to 1.7.

I would only keep the changes in pkg/endpoint/endpoint_status.go plus the version upgrade and remove the deprecated fields from EndpointStatus later on probably once we don't support downgrade to 1.8 (possibly in 1.10)

@tgraf
Copy link
Member Author

tgraf commented Mar 9, 2020

Also, by bumping that version, we will brake downgrade path as that structure will no longer exist if the user wants to downgrade from 1.8 to 1.7.

Why does it need to be bumped? None of the fields are changing. The CRV remains identical. Why would downgrade break? If an upgrade occurred then the old CEP CRD is already registered so it will be unchanged and rollback is possible. The removed fields have been marked as deprecated for a long time and no code should depend on it. If it depends on them, it should break.

@aanm
Copy link
Member

aanm commented Mar 9, 2020

Also, by bumping that version, we will brake downgrade path as that structure will no longer exist if the user wants to downgrade from 1.8 to 1.7.

Why does it need to be bumped? None of the fields are changing. The CRV remains identical. Why would downgrade break? If an upgrade occurred then the old CEP CRD is already registered so it will be unchanged and rollback is possible. The removed fields have been marked as deprecated for a long time and no code should depend on it. If it depends on them, it should break.

I had the impression that we had a CRV for CEP.

@tgraf
Copy link
Member Author

tgraf commented Mar 9, 2020

I had the impression that we had a CRV for CEP.

Thanks for double checking

@tgraf tgraf merged commit a0fcbd7 into master Mar 9, 2020
1.8.0 automation moved this from In progress to Merged Mar 9, 2020
@tgraf tgraf deleted the pr/tgraf/remove-deprecated-endpoint-fields branch March 9, 2020 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tgraf @coveralls @aanm