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

Canal Manifest Fix (Kubernetes >= v1.12.0) #5910

Merged
merged 1 commit into from
Oct 10, 2018
Merged

Canal Manifest Fix (Kubernetes >= v1.12.0) #5910

merged 1 commit into from
Oct 10, 2018

Conversation

gambol99
Copy link
Contributor

So the current canal manifest contains a 'description' field which as far as I can tell from the
API documentation never existed (in apimachinery). Previous versions probably ignored the field but while testing v1.12.1 I noticed canal no longer deployed correctly due to the validation error. I've bumped the versions and removed the field from the manifest; this shouldn't have any impact on those already deployed, but as an alternative we could copy and paste 1.8 manifest, add an exclusion in the bootstrapchannelbuilder.go to use >1.8.0 <=1.12.0 etc and use new manifest for >=1.12.0 (if you get what i know :-))

Post removing the description field the networking works again ..

$ kubectl -n kube-system get no
NAME                                           STATUS    ROLES     AGE       VERSION
ip-10-250-100-205.eu-west-2.compute.internal   Ready     node      7m        v1.12.1
ip-10-250-101-235.eu-west-2.compute.internal   Ready     node      7m        v1.12.1
ip-10-250-31-88.eu-west-2.compute.internal     Ready     master    10m       v1.12.1
ip-10-250-32-8.eu-west-2.compute.internal      Ready     master    10m       v1.12.1
ip-10-250-33-116.eu-west-2.compute.internal    Ready     master    10m       v1.12.1

@gambol99
Canal Manifest Fix (Kubernetes >= v1.12.0)
d1ea6ad 
So the current canal manifest contains a 'description' field which as far as I can tell from the API documentation never existed (in apimachinery). Previous versions probably ignored the field but while testing v1.12.1 I noticed canal no longer deployed correctly due to the validation error. I've bumped the versions and removed the field from the manifest; this shouldn't have any impact on those already deployed, but as an alternative we could copy and paste 1.8 manifest, add an exclusion in the [bootstrapchannelbuilder.go](https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/bootstrapchannelbuilder.go#L685-L699) to use >1.8.0 <=1.12.0 etc and use new manifest for >=1.12.0 (if you get what i know :-))

Post removing the `description` field the networking works again ..
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 10, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 10, 2018
@KashifSaadat
Copy link
Contributor

KashifSaadat commented Oct 10, 2018 •
edited
Loading

No need to have an extra manifest for v1.12+ if it was never valid in the spec for CRDs, so best just to keep what you've already done. :) It looks like the field was present for TPRs so must have just been copied over mistakenly.

Have you validated updating the CRD for v1.8 / v1.10 clusters just to confirm it applies successfully? If so I'll LG 👍

@gambol99
Copy link
Contributor Author

Have you validated updating the CRD for v1.8 / v1.10 clusters just to confirm it applies successfully? If so I'll LG

So the CRD has never had a description field it doesn't exists in apimachinery. So applying in 1.8 it simply ignored the field on the kubectl apply and everything is fine .. I'm guessing in v1.12 validation code has been improved, either at kubectl or api and now the field is highlighted as invalid .. A transition from 1.8 -> 1.10 shouldn't have issues ..

KashifSaadat
KashifSaadat approved these changes Oct 10, 2018
View reviewed changes
Copy link
Contributor

@KashifSaadat KashifSaadat left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @gambol99 !

@KashifSaadat
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gambol99, KashifSaadat

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:
  • OWNERS [KashifSaadat,gambol99]

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

@k8s-ci-robot k8s-ci-robot merged commit f4c1da6 into kubernetes:master Oct 10, 2018
@gambol99 gambol99 deleted the fix_canal_manifest branch October 10, 2018 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KashifSaadat KashifSaadat KashifSaadat approved these changes

@chrisz100 chrisz100 Awaiting requested review from chrisz100

@robinpercy robinpercy Awaiting requested review from robinpercy

Assignees

@KashifSaadat KashifSaadat

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gambol99 @KashifSaadat @k8s-ci-robot