Vendor gardener `1.57.1` and upgrade ginkgo to `v2` #450

AleksandarSavchev · 2022-10-18T12:27:26Z

How to categorize this PR?

/area open-source
/kind enhancement

What this PR does / why we need it:
Vendor gardener v1.57.1
This PR also upgrades ginkgo to v2 and removes unnecessary replace directives from go.mod file.

Which issue(s) this PR fixes:
Fixes #447

Special notes for your reviewer:
The ginkgo upgrade was needed because newer versions of g/g are using v2 and it was causing problems in some of the env tests

Release note:

Dependency `github.com/gardener/gardener` is updated `v1.36.0` -> `v1.57.1`

Dependency `github.com/onsi/ginkgo` is upgraded to `github.com/onsi/ginkgo/v2`

aaronfern

Thanks for the PR @AleksandarSavchev!

A few questions/comments

aaronfern · 2022-10-21T08:09:19Z

config/rbac/role.yaml

  resources:
-  - pods


These permissions are required when deploying druid through kustomize

These permissions were also removed by running make manifests. I am not sure which change caused it. Will have to look into it.

aaronfern · 2022-10-21T08:52:55Z

charts/druid/charts/crds/templates/10-crd-druid.gardener.cloud_etcdcopybackupstasks.yaml

-status:
-  acceptedNames:
-    kind: ""
-    plural: ""
-  conditions: []
-  storedVersions: []


Why remove the status field?

This field was removed by make manifests due to this PR in controller-tools v0.9.0

aaronfern · 2022-10-21T09:27:06Z

pkg/mapper/statefulset_to_etcd.go

@@ -34,7 +35,7 @@ type statefulSetToEtcdMapper struct {
 	cl  client.Client
 }

-func (m *statefulSetToEtcdMapper) Map(obj client.Object) []reconcile.Request {
+func (m *statefulSetToEtcdMapper) Map(ctx context.Context, log logr.Logger, reader client.Reader, obj client.Object) []reconcile.Request {


These newly added parameters are unused in this function

I have added these parameters since they are needed for the new Mapper interface in order not to change the StatefulSetToEtcd() function. The Mapper interface was changed in this PR.

aaronfern

Looks good to me, however a second opinion would be great before I merge this PR
/second-opinion @ishan16696 @abdasgupta

plkokanov

Thanks for the PR! I just have a few minor comments.

plkokanov · 2022-10-27T08:49:54Z

Makefile

@@ -66,11 +63,12 @@ deploy: manifests
 	kustomize build config/default | kubectl apply -f -

 # Generate manifests e.g. CRD, RBAC etc.
+# TODO(AleksandarSavchev): config/rbac/role.yaml has had manual changes that were ruiden by the last command of make manifests. make manifests need a fix.


Suggested change

# TODO(AleksandarSavchev): config/rbac/role.yaml has had manual changes that were ruiden by the last command of make manifests. make manifests need a fix.

# TODO(AleksandarSavchev): config/rbac/role.yaml gets incorrectly overwritten by this make target. Thus the generation of the role is skipped for now until a proper generation and generation check is implemented.

Also could you move this comment right above line 71.

plkokanov · 2022-10-27T08:51:27Z

Makefile

 .PHONY: manifests
 manifests: $(CONTROLLER_GEN)
 	@go generate ./config/crd/bases
 	@find "$(REPO_ROOT)/config/crd/bases" -name "*.yaml" -exec cp '{}' "$(REPO_ROOT)/charts/druid/charts/crds/templates/" \;
-	@controller-gen rbac:roleName=manager-role paths="./controllers/..."
+#   @controller-gen rbac:roleName=manager-role paths="./controllers/..."


Is it safe to skip the entire generation here? @aaronfern

I think it should be okay
This role.yaml file generated is used only when deploying druid using kustomize and not in any other case. So should be okay from my pov

plkokanov · 2022-10-27T08:55:31Z

controllers/etcd_controller.go

@@ -454,7 +453,8 @@ func (r *EtcdReconciler) getPodDisruptionBudgetFromEtcd(etcd *druidv1alpha1.Etcd
 	decoded := &policyv1beta1.PodDisruptionBudget{}
 	pdbPath := getChartPathForPodDisruptionBudget()
 	chartPath := getChartPath()
-	renderedChart, err := r.chartApplier.Render(chartPath, etcd.Name, etcd.Namespace, values)
+	// TODO(AleksandarSavchev): .Render is deprecated. Refactor or adapt code to use RenderEmbeddedFS https://github.com/gardener/gardener/pull/6165


Let's also open an issue for this on gardener/etcd-druid so that we don't forget it

plkokanov · 2022-10-27T08:58:46Z

controllers/etcd_controller.go

+	etcd.Status.Ready = nil
+	etcd.Status.ReadyReplicas = 0
+
+	err := r.Client.Status().Update(ctx, etcd)


Suggested change

err := r.Client.Status().Update(ctx, etcd)

return etcd, r.Client.Status().Update(ctx, etcd)

plkokanov · 2022-10-27T09:08:17Z

controllers/etcd_custodian_controller.go

+
+	return c.Watch(
+		&source.Kind{Type: &appsv1.StatefulSet{}},
+		mapper.EnqueueRequestsFrom(druidmapper.StatefulSetToEtcd(ctx, mgr.GetClient()), mapper.UpdateWithNew, mgr.GetLogger()),


Suggested change

mapper.EnqueueRequestsFrom(druidmapper.StatefulSetToEtcd(ctx, mgr.GetClient()), mapper.UpdateWithNew, mgr.GetLogger()),

mapper.EnqueueRequestsFrom(druidmapper.StatefulSetToEtcd(ctx, mgr.GetClient()), mapper.UpdateWithNew, c.GetLogger()),

aaronfern · 2022-11-04T05:58:43Z

Hi @AleksandarSavchev
Can you please rebase this PR?

unmarshall · 2022-12-14T03:53:57Z

Hi @AleksandarSavchev,
Thanks for the PR. We would like to hold off on this PR for some time till we remove our dependency on /hack/scripts that we source from gardener/gardener into etcd-druid. A lot of dependencies are pulled into the vendor directory when we source /hack/scripts and we hardly use any of those dependencies. Once we remove the dependency on g/g we will also upgrade the g/g version to the latest version.

I hope that is ok with you. Apologies for the delay. I was supposed to leave a comment in this issue earlier but was unable to do it. If you have any concerns then please respond here and we can discuss further.

Best Regards,
Madhav

* Vendor gardener 1.57.1 * Upgrade ginkgo to v2 * linter fixes and added/removed TODOs * Added github username to TODOs * Added review suggestions * Made fixes to PR * Rebase master * squash * remove changes to make manifests

* Vendor gardener 1.57.1 * Upgrade ginkgo to v2 * linter fixes and added/removed TODOs * Added github username to TODOs * Added review suggestions * Made fixes to PR * Rebase master * squash * remove changes to make manifests Co-authored-by: Aleksandar Savchev <[email protected]>

AleksandarSavchev requested a review from a team as a code owner October 18, 2022 12:27

gardener-robot added area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related kind/enhancement Enhancement, improvement, extension needs/review Needs review labels Oct 18, 2022

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 18, 2022

gardener-robot added the size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) label Oct 18, 2022

gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 18, 2022

gardener-robot added the needs/second-opinion Needs second review by someone else label Oct 18, 2022

AleksandarSavchev changed the title ~~Vendor gardener 1.57.0 and upgrade ginkgo to v2~~ Vendor gardener 1.57.1 and upgrade ginkgo to v2 Oct 18, 2022

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 19, 2022

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 20, 2022

aaronfern reviewed Oct 21, 2022

View reviewed changes

AleksandarSavchev requested a review from aaronfern October 25, 2022 08:09

aaronfern reviewed Oct 25, 2022

View reviewed changes

plkokanov suggested changes Oct 27, 2022

View reviewed changes

gardener-robot added the needs/changes Needs (more) changes label Oct 27, 2022

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 27, 2022

AleksandarSavchev force-pushed the upgrade-gardener-and-ginkgo branch from c3d378d to dd4b0a2 Compare November 4, 2022 07:55

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 4, 2022

AleksandarSavchev requested review from plkokanov and aaronfern and removed request for plkokanov and aaronfern November 4, 2022 08:03

gardener-robot removed the needs/second-opinion Needs second review by someone else label Dec 5, 2022

This was referenced Dec 21, 2022

Upgrade k8s.io/* to v0.26.1, sigs.k8s.io/controller-runtime to v0.14.1 gardener/gardener#7248

Merged

Remove CustomReporter from envtest #493

Merged

AleksandarSavchev added 9 commits January 6, 2023 11:13

Vendor gardener 1.57.1

744c74b

Upgrade ginkgo to v2

10c3754

linter fixes and added/removed TODOs

7a43c3b

Added github username to TODOs

270f221

Added review suggestions

0f84ea7

Made fixes to PR

1efcb52

Rebase master

17ee440

squash

0d7678a

remove changes to make manifests

3fde038

AleksandarSavchev force-pushed the upgrade-gardener-and-ginkgo branch from e77cecf to 3fde038 Compare January 6, 2023 09:58

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 6, 2023

gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Jan 6, 2023

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 6, 2023

AleksandarSavchev mentioned this pull request Jan 10, 2023

gardener/gardener and all modules that vendor gardener/gardener require k8s.io/[email protected]+incompatible gardener/gardener#6807

Closed

aaronfern added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 12, 2023

gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 12, 2023

aaronfern merged commit fa55c9b into gardener:master Jan 12, 2023

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jan 12, 2023

unmarshall mentioned this pull request Feb 10, 2023

[BUG] Fix logger time format for improved debugging and issue analysis and ensure structured logging is used. #524

Closed

shreyas-s-rao mentioned this pull request Feb 10, 2023

Use JSON logger with ISO8601 time encoder for controller manager #525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor gardener `1.57.1` and upgrade ginkgo to `v2` #450

Vendor gardener `1.57.1` and upgrade ginkgo to `v2` #450

AleksandarSavchev commented Oct 18, 2022

aaronfern left a comment

aaronfern Oct 21, 2022

AleksandarSavchev Oct 21, 2022

aaronfern Oct 21, 2022

AleksandarSavchev Oct 21, 2022 •

edited

Loading

aaronfern Oct 21, 2022

AleksandarSavchev Oct 21, 2022

aaronfern left a comment

plkokanov left a comment

plkokanov Oct 27, 2022

plkokanov Oct 27, 2022

aaronfern Oct 27, 2022

plkokanov Oct 27, 2022

AleksandarSavchev Oct 27, 2022

plkokanov Oct 27, 2022

plkokanov Oct 27, 2022

aaronfern commented Nov 4, 2022

unmarshall commented Dec 14, 2022

	# TODO(AleksandarSavchev): config/rbac/role.yaml has had manual changes that were ruiden by the last command of make manifests. make manifests need a fix.
	# TODO(AleksandarSavchev): config/rbac/role.yaml gets incorrectly overwritten by this make target. Thus the generation of the role is skipped for now until a proper generation and generation check is implemented.

	err := r.Client.Status().Update(ctx, etcd)
	return etcd, r.Client.Status().Update(ctx, etcd)

	mapper.EnqueueRequestsFrom(druidmapper.StatefulSetToEtcd(ctx, mgr.GetClient()), mapper.UpdateWithNew, mgr.GetLogger()),
	mapper.EnqueueRequestsFrom(druidmapper.StatefulSetToEtcd(ctx, mgr.GetClient()), mapper.UpdateWithNew, c.GetLogger()),

Vendor gardener 1.57.1 and upgrade ginkgo to v2 #450

Vendor gardener 1.57.1 and upgrade ginkgo to v2 #450

Conversation

AleksandarSavchev commented Oct 18, 2022

aaronfern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AleksandarSavchev Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronfern left a comment

Choose a reason for hiding this comment

plkokanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronfern commented Nov 4, 2022

unmarshall commented Dec 14, 2022

Vendor gardener `1.57.1` and upgrade ginkgo to `v2` #450

Vendor gardener `1.57.1` and upgrade ginkgo to `v2` #450

AleksandarSavchev Oct 21, 2022 •

edited

Loading