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

helm: Use batch/v1 apiVersion for CronJob in K8s 1.21+ #16635

Merged
merged 1 commit into from Oct 21, 2021

Conversation

gandro
Copy link
Member

@gandro gandro commented Jun 23, 2021

The CronJob object reached GA in Kubernetes 1.2, thus bumping its api
version from batch/v1beta1 to batch/v1. The old batch/v1beta1 api
version will be removed in Kubernetes 1.25.

This fixes the following Helm warning on Kubernetes 1.21+:

W0623 11:06:59.097404 1094484 warnings.go:67] batch/v1beta1 CronJob is deprecated in v1.21+, unavailable in v1.25+; use batch/v1 CronJob

The first commit is a drive-by fixup for 4cc699d (see commit description).

@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/helm Impacts helm charts and user deployment experience labels Jun 23, 2021
@gandro gandro requested a review from a team as a code owner June 23, 2021 09:11
@gandro gandro requested review from a team and errordeveloper June 23, 2021 09:11
@gandro
Copy link
Member Author

gandro commented Jun 29, 2021

test-me-please

@gandro gandro force-pushed the pr/gandro/helm-fix-cronjob-apiversion branch from 7f20b3a to ccd45ce Compare June 29, 2021 14:38
@gandro
Copy link
Member Author

gandro commented Jun 29, 2021

Tests failed due the branch being too old (rebased CI tests are using flags that don't exist in my branch). Rebasing and restarting Jenkins tests.

@gandro

This comment has been minimized.

@gandro gandro force-pushed the pr/gandro/helm-fix-cronjob-apiversion branch from ccd45ce to be98056 Compare June 29, 2021 16:46
@gandro
Copy link
Member Author

gandro commented Jun 30, 2021

test-me-please

@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jul 1, 2021
@gandro
Copy link
Member Author

gandro commented Jul 1, 2021

Jenkins is green:
image

Will update the commit message to address the typo without re-running Jenkins. Other than that, it is ready to merge as all required tests have passed.

@gandro gandro force-pushed the pr/gandro/helm-fix-cronjob-apiversion branch from be98056 to 8a84c14 Compare July 1, 2021 08:34
@pchaigno
Copy link
Member

pchaigno commented Jul 1, 2021

This pull request doesn't seem to be among the exceptions for the merge freeze, so removing ready-to-merge label.

@pchaigno pchaigno added dont-merge/merge-freeze and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jul 1, 2021
@gandro
Copy link
Member Author

gandro commented Jul 1, 2021

This pull request doesn't seem to be among the exceptions for the merge freeze, so removing ready-to-merge label.

This is because of Travis failing, right?

Edit: Ah, I guess it's because of master having flakes. But I guess the Travis flake I hit should also be fixed before we merge this.

@gandro gandro added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed dont-merge/merge-freeze labels Jul 27, 2021
@gandro
Copy link
Member Author

gandro commented Jul 27, 2021

This needs a rebase, but I'm holding off for #16900 to avoid further conflicts.

Also removing the merge freeze label, as the occuring Travis flake got fixed and my understanding is that master is not frozen anymore.

@stale
Copy link

stale bot commented Aug 28, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 28, 2021
@pchaigno
Copy link
Member

This needs a rebase, but I'm holding off for #16900 to avoid further conflicts.

#16900 was merged 🎉

@stale stale bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 30, 2021
@gandro gandro force-pushed the pr/gandro/helm-fix-cronjob-apiversion branch from 8a84c14 to ef40997 Compare September 14, 2021 10:06
@gandro gandro removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Sep 14, 2021
@gandro gandro force-pushed the pr/gandro/helm-fix-cronjob-apiversion branch from ef40997 to 30024cd Compare September 14, 2021 10:08
@gandro
Copy link
Member Author

gandro commented Sep 14, 2021

test-me-please

Job 'Cilium-PR-K8s-1.20-kernel-4.19' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks ClusterIP Connectivity Checks service.kubernetes.io/service-proxy-name label implementation

Failure Output

FAIL: Request from echo-8fd54d9fd-4f8pz pod to service http://10.100.162.212:80/ failed

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.20-kernel-4.19 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig MonitorAggregation Checks that monitor aggregation flags send notifications

Failure Output

FAIL: Pods are not ready in time: timed out waiting for pods with filter  to be ready: 4m0s timeout expired

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

@gandro
Copy link
Member Author

gandro commented Oct 18, 2021

I believe the tests in question have since been disabled. Rebasing and re-running tests.

@gandro gandro force-pushed the pr/gandro/helm-fix-cronjob-apiversion branch from 30024cd to f6d999c Compare October 18, 2021 08:21
The CronJob object reached GA in Kubernetes 1.21, thus bumping its api
version from `batch/v1beta1` to `batch/v1`. The old `batch/v1beta1` api
version will be removed in Kubernetes 1.25.

This fixes the following Helm warning on Kubernetes 1.21+:

```
W0623 11:06:59.097404 1094484 warnings.go:67] batch/v1beta1 CronJob is deprecated in v1.21+, unavailable in v1.25+; use batch/v1 CronJob
```

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@gandro gandro force-pushed the pr/gandro/helm-fix-cronjob-apiversion branch from f6d999c to 5a9e4c0 Compare October 18, 2021 08:22
@gandro
Copy link
Member Author

gandro commented Oct 18, 2021

@gandro
Copy link
Member Author

gandro commented Oct 20, 2021

I'm marking this ready to merge. It has been open for 4 months now (blocked due to the zero-flake strategy) and is a rather trivial change in the Helm chart. It has been blocked by various CI flakes, many of which have been fixed or disabled, but new ones pop up each new run.

Since all recently failed test suites in the last test run do not contain any Helm-related tests, I am fairly confident that those are also unrelated flakes and that this PR does not introduce any regressions to CI.

@gandro gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 20, 2021
@kkourt kkourt merged commit 90a5d32 into cilium:master Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants