Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

chore: upgrade kubernetes dependencies #1301

Merged

Conversation

sagikazarmark
Copy link
Contributor

@sagikazarmark sagikazarmark commented Oct 14, 2020

Signed-off-by: Mark Sagi-Kazar [email protected]

What this PR does / why we need it:

Upgrades the Kubernetes libraries to 1.19.2
Keeping libraries up to date is generally a good idea. I'm not aware of any major changes that might break backwards compatibility.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

The logr package needed a manual update because of container-runtime. Once 0.7.0 is out, we don't need to manually pin the logr library version.

Some other libraries have also been updated because they are also transitive dependencies.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 14, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @sagikazarmark!

It looks like this is your first PR to kubernetes-sigs/kubefed 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubefed has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 14, 2020
@sagikazarmark
Copy link
Contributor Author

Hm, can't see any obvious reason why the tests are breaking...

Copy link
Contributor

@jimmidyson jimmidyson left a comment

Choose a reason for hiding this comment

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

Thank you for this. Please fix the kind image and, if you don't mind, update to v0.19.3 dependencies, as well as running go mod tidy please to try to keep go.sum in a reasonable state.

scripts/pre-commit.sh Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@sagikazarmark
Copy link
Contributor Author

@jimmidyson thanks for the review. I addressed/replied to your comments.

I actually ran go mod tidy, can you see anything unusual?

@jimmidyson
Copy link
Contributor

@sagikazarmark I was surprised there weren't a lot more removals from go.sum. It's not a blocker though.

@jimmidyson
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2020
@sagikazarmark
Copy link
Contributor Author

I'll take a look at it. My guess is there is a transitive dependency on an earlier Kubernetes version.

@sagikazarmark
Copy link
Contributor Author

Controller runtime for instance still relies on 1.18.6

@jimmidyson
Copy link
Contributor

@sagikazarmark Ah ok that makes sense

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jimmidyson, makkes, sagikazarmark

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

@jimmidyson
Copy link
Contributor

@sagikazarmark Sorry for the bother but would you mind rebasing this against master and dropping the last two commits in this branch (only need the updated kubernetes dependencies)? Hopefully that should sort out the CI issues.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2020
@sagikazarmark
Copy link
Contributor Author

@jimmidyson I've just rebased it, looks like at the same time you did. 😄

@jimmidyson
Copy link
Contributor

Oh I'm sorry @sagikazarmark, I was just trying to help out to get it in, hope that's ok. I'm going to push another commit just to bump the dependencies to latest now they're published, then we can get this merged. Thank you for kicking this upgrade off.

@sagikazarmark
Copy link
Contributor Author

Sure, no worries. 😄 Let me know if I can help. Thanks!

@sagikazarmark
Copy link
Contributor Author

Looks like it fails with the same error 😕

@jimmidyson
Copy link
Contributor

@sagikazarmark My dodgy rebase - I knew I should have waited for you 😉

@sagikazarmark
Copy link
Contributor Author

Hm, I'm not sure, the rebase looks fine to me. Only go.mod and go.sum have changes.

@jimmidyson
Copy link
Contributor

Yeah it seems to be a consistent test failure.

@jimmidyson
Copy link
Contributor

jimmidyson commented Oct 20, 2020

To test the one failing test you can run (after spinning up some clusters and deploying kubefed - scripts/create-clusters.sh and make deploy.kind should help I think):

$ make bin/e2e
$ bin/e2e -kubeconfig=${HOME}/.kube/config -ginkgo.v -single-call-timeout=1m -ginkgo.trace -ginkgo.randomizeAllSpecs -ginkgo.focus="deployments.apps.* should be created, read, updated and deleted successfully"

I'm also looking at this btw so hope we can get this fixed quickly and merged.

@sagikazarmark
Copy link
Contributor Author

I can see a bunch of ERROR: Cluster object is not as expected errors caused by an extra "deployment.kubernetes.io/revision": "1" annotation in the object.

@jimmidyson
Copy link
Contributor

Yeah it looks like a bug in the jsonpatch upgrade (test below).

package a

import (
	"encoding/json"
	"fmt"
	"testing"

	jsonpatch "github.com/evanphx/json-patch"
	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

	"sigs.k8s.io/kubefed/pkg/controller/util"
)

func TestPatch(t *testing.T) {
	orig := &unstructured.Unstructured{
		Object: map[string]interface{}{
			"metadata": map[string]interface{}{
				"annotations": map[string]interface{}{
					"deployment.kubernetes.io/revision": "1",
				},
			},
		},
	}
	origJSON, _ := orig.MarshalJSON()

	overrides := util.ClusterOverrides{{
		Op:   "add",
		Path: "/metadata/annotations",
		Value: map[string]string{
			"foo": "bar",
		},
	}, {
		Path: "/metadata/annotations/foo",
		Op:   "remove",
	}}
	jsonPatchBytes, _ := json.Marshal(overrides)
	patch, _ := jsonpatch.DecodePatch(jsonPatchBytes)

	patchedObjectJSONBytes, _ := patch.Apply(origJSON)
	fmt.Println(string(patchedObjectJSONBytes))
}

@jimmidyson
Copy link
Contributor

Or potentially a fix in the JSONPatch library... I think this is it tbh.

@jimmidyson
Copy link
Contributor

Ah yes this is because the override is wrong... fixing now.

@jimmidyson
Copy link
Contributor

jimmidyson commented Oct 20, 2020

OK fixed. The override was actually invalid...

@jimmidyson
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2020
@jimmidyson
Copy link
Contributor

Thanks @sagikazarmark!

@k8s-ci-robot k8s-ci-robot merged commit 5e7eb12 into kubernetes-retired:master Oct 21, 2020
@sagikazarmark sagikazarmark deleted the upgrade-dependencies branch October 21, 2020 08:45
@sagikazarmark
Copy link
Contributor Author

Thanks for the help @jimmidyson

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants