Fixes node label error handling #1892

jayanthvn · 2022-02-28T21:10:57Z

What type of PR is this?
bug

Which issue does this PR fix:
Fixes #1887

What does this PR do / Why do we need it:
Added error handling and node patch instead of update.

Using MergeFrom instead of StrategicMergeFrom, @M00nF1sh any preference here?

/ The difference between MergeFrom and StrategicMergeFrom lays in the handling of modified list fields.
// When using MergeFrom, existing lists will be completely replaced by new lists.
// When using StrategicMergeFrom, the list field's `patchStrategy` is respected if specified in the API type,

Ref: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/patch.go#L164

This will need patch permission for nodes. Will evaluate if we can remove update from nodes.

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
#1887

Testing done on this change:

Custom networking -

[varavaj@dev-dsk-varavaj-2b-3c80898a amazon-vpc-cni-k8s]$ kubectl get pods -n kube-system -owide
NAME                       READY   STATUS    RESTARTS   AGE     IP              NODE                                          NOMINATED NODE   READINESS GATES
aws-node-524nl             1/1     Running   0          4h30m   192.168.76.72   ip-192-168-76-72.us-west-2.compute.internal   <none>           <none>
coredns-6548845887-knt4k   1/1     Running   0          4h32m   100.64.91.181   ip-192-168-76-72.us-west-2.compute.internal   <none>           <none>
coredns-6548845887-svnp6   1/1     Running   0          4h32m   100.64.95.229   ip-192-168-76-72.us-west-2.compute.internal   <none>           <none>
kube-proxy-vnftj           1/1     Running   0          4h30m   192.168.76.72   ip-192-168-76-72.us-west-2.compute.internal   <none>           <none>

[varavaj@dev-dsk-varavaj-2b-3c80898a amazon-vpc-cni-k8s]$ kubectl get pods -owide
NAME                                READY   STATUS    RESTARTS   AGE     IP              NODE                                          NOMINATED NODE   READINESS GATES
nginx-deployment-66b6c48dd5-8skfv   1/1     Running   0          4h32m   100.64.90.119   ip-192-168-76-72.us-west-2.compute.internal   <none>           <none>
nginx-deployment-66b6c48dd5-nn7d7   1/1     Running   0          4h32m   100.64.84.166   ip-192-168-76-72.us-west-2.compute.internal   <none>           <none>
nginx-deployment-66b6c48dd5-wk8lk   1/1     Running   0          4h32m   100.64.93.93    ip-192-168-76-72.us-west-2.compute.internal   <none>           <none>

{"level":"info","ts":"2022-03-04T02:05:16.663Z","caller":"ipamd/ipamd.go:541","msg":"Get Node Info for: ip-192-168-76-72.us-west-2.compute.internal"}
{"level":"debug","ts":"2022-03-04T02:05:16.764Z","caller":"eniconfig/eniconfig.go:136","msg":"Using ENI_CONFIG_LABEL_DEF failure-domain.beta.kubernetes.io/zone"}
{"level":"debug","ts":"2022-03-04T02:05:16.764Z","caller":"retry/util.go:51","msg":"Node found \"ip-192-168-76-72.us-west-2.compute.internal\" - labels - '\\x12'"}
{"level":"debug","ts":"2022-03-04T02:05:16.779Z","caller":"retry/util.go:51","msg":"Updated node ip-192-168-76-72.us-west-2.compute.internal with label \"vpc.amazonaws.com/eniConfig\": \"us-west-2c\""}

{"level":"info","ts":"2022-03-04T02:05:19.281Z","caller":"eniconfig/eniconfig.go:73","msg":"Get Node Info for: ip-192-168-76-72.us-west-2.compute.internal"}
{"level":"debug","ts":"2022-03-04T02:05:19.281Z","caller":"eniconfig/eniconfig.go:136","msg":"Using ENI_CONFIG_LABEL_DEF failure-domain.beta.kubernetes.io/zone"}
{"level":"info","ts":"2022-03-04T02:05:19.281Z","caller":"ipamd/ipamd.go:837","msg":"Found ENI Config Name: us-west-2c"}
{"level":"info","ts":"2022-03-04T02:05:19.382Z","caller":"ipamd/ipamd.go:811","msg":"ipamd: using custom network config: [], subnet-06cac0e2cf3843ec7"}
{"level":"info","ts":"2022-03-04T02:05:19.382Z","caller":"awsutils/awsutils.go:730","msg":"Using a custom network config for the new ENI"}

Fixed the below log in next commit -

{"level":"warn","ts":"2022-03-04T02:05:19.382Z","caller":"awsutils/awsutils.go:730","msg":"No custom networking security group found, will use the node's primary ENI's SG: [%!s(*string=0xc000078400) %!s(*string=0xc000078410)]"}

API server calls -

At 07:26:14.789Z upgraded from 1.7.10 to image with fix. API server calls are in sync with what we tested in 1.8.0

{"level":"debug","ts":"2022-03-04T07:26:07.036Z","caller":"sdk/informer-sync.go:88","msg":"Handle corev1.Node: ip-192-168-76-72.us-west-2.compute.internal, map[node.alpha.kubernetes.io/ttl:0 volumes.kubernetes.io/controller-managed-attach-detach:true], map[alpha.eksctl.io/cluster-name:dev-cluster alpha.eksctl.io/nodegroup-name:nodegroup beta.kubernetes.io/arch:amd64 beta.kubernetes.io/instance-type:t3.small beta.kubernetes.io/os:linux eks.amazonaws.com/capacityType:ON_DEMAND eks.amazonaws.com/nodegroup:nodegroup eks.amazonaws.com/nodegroup-image:ami-02861c26f5cf11d24 eks.amazonaws.com/sourceLaunchTemplateId:lt-0dd642d5c82ff5a73 eks.amazonaws.com/sourceLaunchTemplateVersion:1 failure-domain.beta.kubernetes.io/region:us-west-2 failure-domain.beta.kubernetes.io/zone:us-west-2c kubernetes.io/arch:amd64 kubernetes.io/hostname:ip-192-168-76-72.us-west-2.compute.internal kubernetes.io/os:linux node.kubernetes.io/instance-type:t3.small topology.kubernetes.io/region:us-west-2 topology.kubernetes.io/zone:us-west-2c vpc.amazonaws.com/eniConfig:us-west-2c]"}
{"level":"debug","ts":"2022-03-04T07:26:07.044Z","caller":"sdk/informer-sync.go:88","msg":"Handle ENIConfig Add/Update: us-west-2a, [], subnet-0eeb2ea845de1c8d3"}
{"level":"debug","ts":"2022-03-04T07:26:07.044Z","caller":"sdk/informer-sync.go:88","msg":"Handle ENIConfig Add/Update: us-west-2b, [], subnet-0747217e1d86e6510"}
{"level":"debug","ts":"2022-03-04T07:26:07.044Z","caller":"sdk/informer-sync.go:88","msg":"Handle ENIConfig Add/Update: us-west-2c, [], subnet-06cac0e2cf3843ec7"}
{"level":"info","ts":"2022-03-04T07:26:14.789Z","caller":"aws-k8s-agent/main.go:27","msg":"Starting L-IPAMD   ..."}

SGPP -

[varavaj@dev-dsk-varavaj-2b-3c80898a amazon-vpc-cni-k8s]$ kubectl get nodes -oyaml | grep 'vpc.amazonaws.com/has-trunk-attached: "'
      vpc.amazonaws.com/has-trunk-attached: "true"
      vpc.amazonaws.com/has-trunk-attached: "true"

[varavaj@dev-dsk-varavaj-2b-3c80898a amazon-vpc-cni-k8s]$ kubectl get pods -owide
NAME                              READY   STATUS    RESTARTS   AGE   IP               NODE                                           NOMINATED NODE   READINESS GATES
branch-eni-app-854559894b-496sx   1/1     Running   0          19m   192.168.78.198   ip-192-168-89-129.us-west-2.compute.internal   <none>           <none>
branch-eni-app-854559894b-6t8cm   1/1     Running   0          19m   192.168.90.233   ip-192-168-89-129.us-west-2.compute.internal   <none>           <none>
branch-eni-app-854559894b-7tdfk   1/1     Running   0          19m   192.168.69.42    ip-192-168-89-129.us-west-2.compute.internal   <none>           <none>
branch-eni-app-854559894b-lnzsx   1/1     Running   0          19m   192.168.89.77    ip-192-168-89-129.us-west-2.compute.internal   <none>           <none>
branch-eni-app-854559894b-mm5hr   1/1     Running   0          19m   192.168.64.154   ip-192-168-89-129.us-west-2.compute.internal   <none>           <none>

kubectl describe pod branch-eni-app-854559894b-496sx | grep -B1 eniId
              vpc.amazonaws.com/pod-eni:
                [{"eniId":"eni-0bbb02b3cbdfd5bf7","ifAddress":"0a:e5:6f:88:d3:97","privateIp":"192.168.78.198","vlanId":4,"subnetCidr":"192.168.64.0/19"}]

Automation added to e2e:

No

Will this PR introduce any new dependencies?:

No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No

Does this change require updates to the CNI daemonset config files to work?:

Yes, need to add patch permission for nodes.

Does this PR introduce any user-facing change?:

No

Patch permissions needs to be added with latest controller runtime, update is treated as patch.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

M00nF1sh

i prefer strategicMergePatch over mergePatch given it saves payload and we don't need conflict check for this specific usecase.

jayanthvn · 2022-02-28T21:33:02Z

Sure sounds good. I am updating the PR.

jayanthvn · 2022-03-03T22:12:16Z

i prefer strategicMergePatch over mergePatch given it saves payload and we don't need conflict check for this specific usecase.

Updated with strategicMerge.

…8s into merge_two_way

jayanthvn · 2022-03-04T14:30:41Z

Reviewers - I'm looking into UT cases.

…8s into merge_two_way

jayanthvn · 2022-03-04T15:35:18Z

Reviewers - I'm looking into UT cases.

UTs updated.

pkg/ipamd/ipamd.go

…8s into merge_two_way

M00nF1sh

/lgtm
/approve

jayanthvn added 2 commits February 28, 2022 20:53

Fixes node label error handling

b4d5036

Adding patch permission for nodes

370f679

jayanthvn requested a review from M00nF1sh February 28, 2022 21:10

jayanthvn requested a review from a team as a code owner February 28, 2022 21:10

Merge branch 'master' into merge_two_way

ba0bb9d

M00nF1sh approved these changes Feb 28, 2022

View reviewed changes

Use startegic merge

7f2dfd7

jayanthvn added 3 commits March 3, 2022 14:12

Merge branch 'master' into merge_two_way

4ee7b3c

Update go.sum

ea8b00e

Merge branch 'merge_two_way' of github.com:jayanthvn/amazon-vpc-cni-k…

7053792

…8s into merge_two_way

jayanthvn requested a review from achevuru March 3, 2022 23:02

jayanthvn added 2 commits March 3, 2022 23:47

Update go.sum for metrics

6429ee7

Merge branch 'master' into merge_two_way

349146c

jayanthvn added 2 commits March 4, 2022 15:33

update UTs

c85dd6a

Merge branch 'merge_two_way' of github.com:jayanthvn/amazon-vpc-cni-k…

1d1016c

…8s into merge_two_way

jayanthvn added 2 commits March 7, 2022 10:59

Merge branch 'master' into merge_two_way

052308d

Merge branch 'master' into merge_two_way

408a816

M00nF1sh reviewed Mar 8, 2022

View reviewed changes

pkg/ipamd/ipamd.go Outdated Show resolved Hide resolved

jayanthvn added 2 commits March 8, 2022 00:44

minor nit

89a60e2

Merge branch 'merge_two_way' of github.com:jayanthvn/amazon-vpc-cni-k…

b47c30c

…8s into merge_two_way

M00nF1sh approved these changes Mar 8, 2022

View reviewed changes

jayanthvn merged commit 611d880 into aws:master Mar 8, 2022

jayanthvn deleted the merge_two_way branch March 8, 2022 01:02

jayanthvn mentioned this pull request Mar 10, 2022

Warning message is displaying security groups' memory address, instead of name/ID #1922

Closed

M00nF1sh mentioned this pull request Apr 11, 2022

revert to use update for node label update #1959

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes node label error handling #1892

Fixes node label error handling #1892

jayanthvn commented Feb 28, 2022 •

edited

Loading

M00nF1sh left a comment

jayanthvn commented Feb 28, 2022

jayanthvn commented Mar 3, 2022

jayanthvn commented Mar 4, 2022 •

edited

Loading

jayanthvn commented Mar 4, 2022

M00nF1sh left a comment

Fixes node label error handling #1892

Fixes node label error handling #1892

Conversation

jayanthvn commented Feb 28, 2022 • edited Loading

M00nF1sh left a comment

Choose a reason for hiding this comment

jayanthvn commented Feb 28, 2022

jayanthvn commented Mar 3, 2022

jayanthvn commented Mar 4, 2022 • edited Loading

jayanthvn commented Mar 4, 2022

M00nF1sh left a comment

Choose a reason for hiding this comment

jayanthvn commented Feb 28, 2022 •

edited

Loading

jayanthvn commented Mar 4, 2022 •

edited

Loading