Fix calico-node in etcd mode

[olevitt]

What type of PR is this?
/kind bug

What this PR does / why we need it:

This PR fixes a regression introduced by #10177 which occurs when running in calico etcd mode.
More details on #10436.

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

Special notes for your reviewer:
This PR fixes a bug that is critical for users running calico in etcd mode. It may be useful to add this to the know issues for v2.23.0

Does this PR introduce a user-facing change?:

Fix calico node when running in etcd mode

Thanks for maintaining this great project 😍

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: olevitt / name: Olivier Levitt (1916587, 64ba3b1)

[k8s-ci-robot]

Hi @olevitt. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mzaian]

Hello @olevitt

Thanks for your PR. I would suggest to use the default PR template also to have the bug note automatically generated for the next release.

/ok-to-test

[olevitt]

@mzaian Thanks ! I changed the PR text to be in line with the default template. Feel free to amend anything you think may be improved.

[cyclinder]

I'm curious why we haven't run into this issue before. Actually, this PR is just moving the config file to configMap. No matter which mode Calico is running in, we need KUBERNETES_NODE_NAME, I believe there was the same issue before, your fix makes sense, thanks.

updated:

refer to https://github.com/projectcalico/calico/blob/master/manifests/calico-etcd.yaml, In etcd mode, we should configure etcd_endpoints, it look like we don't need KUBERNETES_NODE_NAME, I'm curious if your cluster is normal before it is upgraded, can you show the contents of the /etc/cni/net.d/10-calico.conflist?

/lgtm

[cyclinder]

revert this changes and add follwing env:

           # The location of the etcd cluster.
            - name: ETCD_ENDPOINTS
              valueFrom:
                configMapKeyRef:
                  name: calico-config
                  key: etcd_endpoints

[cyclinder]

You can test it in your cluster and see if works,

[cyclinder]

revert the changes, too

[cyclinder]

ping @olevitt, Can you please respond to me?

/hold

[olevitt]

@cyclinder sorry for the delay.
My understanding on this is that the issue is all about having nodename properly set on each node which was not the case for me after upgrading and I think that's due to #10177 using {{ calico_baremetal_nodename }} resulting in the same name (first controlplane) on all nodes.
ETCD_ENDPOINTS looks fine and are, to my understanding, not related to this bug.

Here is my /etc/cni/net.d/10-calico.conflist (on a random node), after applying the fix in this PR (before applying the fix I had nodename set as the first controlplane).

root@boss4:~# cat /etc/cni/net.d/10-calico.conflist
{
  "name": "k8s-pod-network",
  "cniVersion":"0.3.1",
  "plugins":[
    {
                            "nodename": "boss4",
                            "type": "calico",
        "log_level": "info",
                  "log_file_path": "/var/log/calico/cni/cni.log",
                            "etcd_endpoints": "https://192.168.254.134:2379,https://192.168.254.135:2379,https://192.168.254.136:2379",
        "etcd_cert_file": "/etc/calico/certs/cert.crt",
        "etcd_key_file": "/etc/calico/certs/key.pem",
        "etcd_ca_cert_file": "/etc/calico/certs/ca_cert.crt",
                            "ipam": {
          "type": "calico-ipam",
                        "assign_ipv4": "true"
        },
                                                "policy": {
          "type": "k8s"
        },
                            "kubernetes": {
          "kubeconfig": "/etc/cni/net.d/calico-kubeconfig"
        }
    },
    {
      "type":"portmap",
      "capabilities": {
        "portMappings": true
      }
    },
    {
      "type":"bandwidth",
      "capabilities": {
        "bandwidth": true
      }
    }
  ]
}

[cyclinder]

thanks for the clarification @olevitt.

calico_datastore Is it kdd on your cluster? I think there was the same issue before. I think your change applies to all cases. But look at https://github.com/projectcalico/calico/blob/master/manifests/calico-etcd.yaml, It seems we don't need K8S_NODE_NAME in calico etcd mode, I hope our calico manifests to be as consistent as possible with calico upstream.

[olevitt]

@cyclinder calico_datastore is etcd in my cluster and have been since it's creation (almost 3 years ago).
I don't have enough knowledge on calico to say but having a wrong nodename in the configuration definitely make it assume that node configuration and go crazy. That's the bug I encountered when upgrading. Maybe nodename is optional in etcd mode but is used when present ? I may test what happens when no nodename is set in etcd mode

[cyclinder]

Maybe nodename is optional in etcd mode but is used when present ?

I think calico doesn't use nodeName in etcd mode, At least, https://github.com/projectcalico/calico/blob/master/manifests/calico-etcd.yaml don't set it.

I may test what happens when no nodename is set in etcd mode

thanks! Can you add ETCD_ENDPONINTS ENV to your calico-node? just like https://github.com/projectcalico/calico/blob/master/manifests/calico-etcd.yaml

[floryut]

Maybe nodename is optional in etcd mode but is used when present ?

I think calico doesn't use nodeName in etcd mode, At least, https://github.com/projectcalico/calico/blob/master/manifests/calico-etcd.yaml don't set it.

I may test what happens when no nodename is set in etcd mode

thanks! Can you add ETCD_ENDPONINTS ENV to your calico-node? just like https://github.com/projectcalico/calico/blob/master/manifests/calico-etcd.yaml

@olevitt Can you check @cyclinder comments ?

As soon as it's ok the hold flag can be removed 👍

[cyclinder]

friendly ping ^^^ @olevitt

[olevitt]

Once again sorry for the delay. I got to test and it seems that, as you said, removing nodename and adding ETCD_ENDPONINTS ENV to the install-cni init container works, at least on my setup. I will rewrite this PR according to your suggestions

@cyclinder Done.
Not sure if I should add other ETCD env variables such as ETCD_CA_CERT_FILE ?

[cyclinder]

Thanks @olevitt

Not sure if I should add other ETCD env variables such as ETCD_CA_CERT_FILE?

Yes, But we already added these variables. Your changes look good now, Thanks for your work👍

/hold cancel
/lgtm

[VannTen]

Chiming in, as our clusters also use calico in etcd mode.

Maybe we could add that deployment mode to the CI test, wdyt @floryut
That kind of breakage seems serious enough, and etcd mode used to be the default, so I suppose there is a number of existing clusters still around.

[floryut]

Chiming in, as our clusters also use calico in etcd mode.

Maybe we could add that deployment mode to the CI test, wdyt @floryut That kind of breakage seems serious enough, and etcd mode used to be the default, so I suppose there is a number of existing clusters still around.

Never hurt to add more cases to the CI tbh 👍

[VannTen]

Once we confirm that the test in the linked PR catch the problem, could you add it to your PR (by rebasing or cherry-picking, whatever). That way we can be sure it does fix the problem and we'll catch future stuff.

[olevitt]

Sure, ping me when / if I need to rebase this PR.
Btw, maintainers have permissions to edit / commit to olevitt/kubespray. If anyone wants / needs to do so, feel free to.

[VannTen]

Yeah but I think Github handles that with write permission on the upstream, which I don't have, not sure who does. Most merge go through the tide bot, so it's rarely needed (to have direct write permissions I mean).

[yankay]

Thanks @olevitt @VannTen @cyclinder 👍

merge the PR first so the test case can be added later :-)

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cyclinder, olevitt, yankay

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 [yankay]

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

[jonathansloman]

Hi - will this fix also be added to a 2.23.2 release? The issue is preventing us from upgrading our cluster, and kubespray documentation specifies not to skip releases when upgrading (ie, we shouldn't go from 2.22 directly to 2.24), so we need a working 2.23 to give us an upgrade path.

Thank you.

[VannTen]

I plan to backport it to release-2.23 along with the other necessary PRs (#10725 #10722 ) because my client also needs it, however I'm quite busy and not yet back to work so if you can (backport it) go for it. I don't think there is any complicated conflicts to resolve.