From 6e95bccc96c4a98bdbb349e42b91ef7bdc009a7e Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Wed, 18 Aug 2021 18:05:56 +0530 Subject: [PATCH 1/4] packet: fix panic on entry in nil map In the latest version of cluster-autoscaler (cloudprovider: packet), the code panics and the pods go into CrashLoopBackoff due to an entry assignment on a nil map. This commit fixes that by initializing the ConfigFile instance. I believe this situation is created when the config file doesn't contain any information about the nodepool and also `default` config is not present, but this does not take care the use case of when `Global` section is defined in the config file. Below is the error reproduced when `[Global]` is used in the config file. ``` panic: assignment to entry in nil map goroutine 131 [running]: k8s.io/autoscaler/cluster-autoscaler/cloudprovider/packet.createPacketManagerRest(0x44cf260, 0xc00085e448, 0xc000456670, 0x1, 0x1, 0x0, 0x0, 0x0, 0x3fe0000000000000, 0x3fe0000000000000, ...) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go:307 +0xaca k8s.io/autoscaler/cluster-autoscaler/cloudprovider/packet.createPacketManager(0x44cf260, 0xc00085e448, 0xc000456670, 0x1, 0x1, 0x0, 0x0, 0x0, 0x3fe0000000000000, 0x3fe0000000000000, ...) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/cloudprovider/packet/packet_manager.go:64 +0x179 k8s.io/autoscaler/cluster-autoscaler/cloudprovider/packet.BuildPacket(0x3fe0000000000000, 0x3fe0000000000000, 0x1bf08eb000, 0x1176592e000, 0xa, 0x0, 0x4e200, 0x0, 0x186a0000000000, 0x0, ...) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/cloudprovider/packet/packet_cloud_provider.go:164 +0xe5 k8s.io/autoscaler/cluster-autoscaler/cloudprovider/builder.buildCloudProvider(0x3fe0000000000000, 0x3fe0000000000000, 0x1bf08eb000, 0x1176592e000, 0xa, 0x0, 0x4e200, 0x0, 0x186a0000000000, 0x0, ...) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/cloudprovider/builder/builder_all.go:91 +0x31f k8s.io/autoscaler/cluster-autoscaler/cloudprovider/builder.NewCloudProvider(0x3fe0000000000000, 0x3fe0000000000000, 0x1bf08eb000, 0x1176592e000, 0xa, 0x0, 0x4e200, 0x0, 0x186a0000000000, 0x0, ...) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/cloudprovider/builder/cloud_provider_builder.go:45 +0x1e6 k8s.io/autoscaler/cluster-autoscaler/core.initializeDefaultOptions(0xc0013876e0, 0x452ef01, 0xc000d80e20) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/core/autoscaler.go:101 +0x2fd k8s.io/autoscaler/cluster-autoscaler/core.NewAutoscaler(0x3fe0000000000000, 0x3fe0000000000000, 0x1bf08eb000, 0x1176592e000, 0xa, 0x0, 0x4e200, 0x0, 0x186a0000000000, 0x0, ...) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/core/autoscaler.go:65 +0x43 main.buildAutoscaler(0xc000313600, 0xc000d00000, 0x4496df, 0x7f9c7b60b4f0) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/main.go:337 +0x368 main.run(0xc00063e230) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/main.go:343 +0x39 main.main.func2(0x453b440, 0xc00029d380) /gopath/src/k8s.io/autoscaler/cluster-autoscaler/main.go:447 +0x2a created by k8s.io/client-go/tools/leaderelection.(*LeaderElector).Run /gopath/src/k8s.io/autoscaler/cluster-autoscaler/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:207 +0x113 ``` Signed-off-by: Imran Pochi --- .../cloudprovider/packet/packet_manager_rest.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go b/cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go index 772e86799c8e..aa527f078524 100644 --- a/cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go +++ b/cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go @@ -292,7 +292,12 @@ func Contains(a []string, x string) bool { // createPacketManagerRest sets up the client and returns // an packetManagerRest. func createPacketManagerRest(configReader io.Reader, discoverOpts cloudprovider.NodeGroupDiscoveryOptions, opts config.AutoscalingOptions) (*packetManagerRest, error) { - var cfg ConfigFile + // Initialize ConfigFile instance + cfg := ConfigFile{ + DefaultNodegroupdef: ConfigNodepool{}, + Nodegroupdef: map[string]*ConfigNodepool{}, + } + if configReader != nil { if err := gcfg.ReadInto(&cfg, configReader); err != nil { klog.Errorf("Couldn't read config: %v", err) From 0041a393ec6589847efe430fe021a19650ae38de Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Wed, 18 Aug 2021 18:11:13 +0530 Subject: [PATCH 2/4] packet: Consider the string prefix equinixmetal:// This commit adds another string prefix to consider `equinixmetal://` along with the existing prefix `packet://`. When K8s API is queried to get providerID from Node Spec, some machines return `packet://`, whereas some return `equinixmetal://`, this creates error as the string is not trimmed properly and hence results in a 404 when an untrimmed string is queried to Equinix Metal API for device information. Signed-off-by: Imran Pochi --- .../packet/packet_manager_rest.go | 37 +++++++++++++++++-- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go b/cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go index aa527f078524..121f9e3f3fb6 100644 --- a/cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go +++ b/cluster-autoscaler/cloudprovider/packet/packet_manager_rest.go @@ -48,6 +48,8 @@ import ( const ( userAgent = "kubernetes/cluster-autoscaler/" + version.ClusterAutoscalerVersion expectedAPIContentTypePrefix = "application/json" + packetPrefix = "packet://" + equinixMetalPrefix = "equinixmetal://" ) type instanceType struct { @@ -436,7 +438,11 @@ func (mgr *packetManagerRest) NodeGroupForNode(labels map[string]string, nodeId if nodegroup, ok := labels["pool"]; ok { return nodegroup, nil } - device, err := mgr.getPacketDevice(context.TODO(), strings.TrimPrefix(nodeId, "packet://")) + + trimmedNodeId := strings.TrimPrefix(nodeId, packetPrefix) + trimmedNodeId = strings.TrimPrefix(trimmedNodeId, equinixMetalPrefix) + + device, err := mgr.getPacketDevice(context.TODO(), trimmedNodeId) if err != nil { return "", fmt.Errorf("Could not find group for node: %s %s", nodeId, err) } @@ -595,9 +601,30 @@ func (mgr *packetManagerRest) getNodes(nodegroup string) ([]string, error) { nodes := []string{} + // This bit of code along with the switch statement, checks if the CCM installed on the cluster is + // `packet-ccm` or `cloud-provider-equinix-metal`. The reason its important to check because depending + // on the CCM installed, the prefix in providerID of K8s Node spec differs from either `packet://` or + // `equinixmetal://`. This is now needed as `packet-ccm` is now deprecated and renamed in favor of + // `cloud-provider-equinix-metal`. + // This code checks if the INSTALLED_CCM env var is set or not. If set to `cloud-provider-equinix-metal`, + // the prefix is set to `equinixmetal://` and any other case the prefix is `packet://`. + // At a later point in time, there would be a need to make `equinixmetal://` prefix as the default or do away + // with `packet://` prefix entirely. This should happen presumably when the packet code in autoscaler is + // renamed from packet to equinixmetal. + prefix := packetPrefix + + switch installedCCM := os.Getenv("INSTALLED_CCM"); installedCCM { + case "packet-ccm": + prefix = packetPrefix + case "cloud-provider-equinix-metal": + prefix = equinixMetalPrefix + default: + klog.V(3).Info("Unrecognized value: expected INSTALLED_CCM to be either `packet-ccm` or `cloud-provider-equinix-metal`, using default: `packet-ccm`") + } + for _, d := range devices.Devices { if Contains(d.Tags, "k8s-cluster-"+mgr.getNodePoolDefinition(nodegroup).clusterName) && Contains(d.Tags, "k8s-nodepool-"+nodegroup) { - nodes = append(nodes, fmt.Sprintf("packet://%s", d.ID)) + nodes = append(nodes, fmt.Sprintf("%s%s", prefix, d.ID)) } } @@ -665,11 +692,15 @@ func (mgr *packetManagerRest) deleteNodes(nodegroup string, nodes []NodeRef, upd klog.Infof("Checking device %v", d) if Contains(d.Tags, "k8s-cluster-"+mgr.getNodePoolDefinition(nodegroup).clusterName) && Contains(d.Tags, "k8s-nodepool-"+nodegroup) { klog.Infof("nodegroup match %s %s", d.Hostname, n.Name) + + trimmedName := strings.TrimPrefix(n.Name, packetPrefix) + trimmedName = strings.TrimPrefix(trimmedName, equinixMetalPrefix) + switch { case d.Hostname == n.Name: klog.V(1).Infof("Matching Packet Device %s - %s", d.Hostname, d.ID) errList = append(errList, mgr.deleteDevice(ctx, nodegroup, d.ID)) - case fakeNode && strings.TrimPrefix(n.Name, "packet://") == d.ID: + case fakeNode && trimmedName == d.ID: klog.V(1).Infof("Fake Node %s", d.ID) errList = append(errList, mgr.deleteDevice(ctx, nodegroup, d.ID)) } From 87beac1af7ac11fe9b2b8b23f38c4728600d8d84 Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Wed, 18 Aug 2021 18:40:49 +0530 Subject: [PATCH 3/4] packet: make controller node label configurable Currently the label to identify controller/master node is hard coded to `node-role.kubernetes.io/master`. There have been some conversations centered around replacing the label with `node-role.kubernetes.io/control-plane`. In [Lokomotive](github.com/kinvolk/lokomotive), the label to identify the controller/master node is `node.kubernetes.io/master`, the reasons for this is mentioned in this [issue](https://github.com/kinvolk/lokomotive/issues/227) This commit makes the label configurable by setting an env variable in the deployment `CONTROLLER_NODE_IDENTIFIER_LABEL`, if set then the value in the env variable is used for identifying controller/master nodes, if not set/passed, then the existing behaviour is followed choosing the existing label. Signed-off-by: Imran Pochi --- .../cloudprovider/packet/packet_cloud_provider.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/cluster-autoscaler/cloudprovider/packet/packet_cloud_provider.go b/cluster-autoscaler/cloudprovider/packet/packet_cloud_provider.go index 32e28e7e6ad8..ca59a84f3d23 100644 --- a/cluster-autoscaler/cloudprovider/packet/packet_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/packet/packet_cloud_provider.go @@ -37,6 +37,11 @@ const ( ProviderName = "packet" // GPULabel is the label added to nodes with GPU resource. GPULabel = "cloud.google.com/gke-accelerator" + // DefaultControllerNodeLabelKey is the label added to Master/Controller to identify as + // master/controller node. + DefaultControllerNodeLabelKey = "node-role.kubernetes.io/master" + // ControllerNodeIdentifierEnv is the string for the environment variable. + ControllerNodeIdentifierEnv = "PACKET_CONTROLLER_NODE_IDENTIFIER_LABEL" ) var ( @@ -94,7 +99,13 @@ func (pcp *packetCloudProvider) AddNodeGroup(group packetNodeGroup) { // // Since only a single node group is currently supported, the first node group is always returned. func (pcp *packetCloudProvider) NodeGroupForNode(node *apiv1.Node) (cloudprovider.NodeGroup, error) { - if _, found := node.ObjectMeta.Labels["node-role.kubernetes.io/master"]; found { + controllerNodeLabel := os.Getenv(ControllerNodeIdentifierEnv) + if controllerNodeLabel == "" { + klog.V(3).Infof("env %s not set, using default: %s", ControllerNodeIdentifierEnv, DefaultControllerNodeLabelKey) + controllerNodeLabel = DefaultControllerNodeLabelKey + } + + if _, found := node.ObjectMeta.Labels[controllerNodeLabel]; found { return nil, nil } nodeGroupId, err := pcp.packetManager.NodeGroupForNode(node.ObjectMeta.Labels, node.Spec.ProviderID) From 8e6f109dabdf346dffa95f5a9d7bd211bafb3a23 Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Thu, 19 Aug 2021 20:38:35 +0530 Subject: [PATCH 4/4] packet: Add documentation regarding new env variable Adds documentation regarding env variables introduced: - PACKET_CONTROLLER_NODE_IDENTIFIER_LABEL - INSTALLED_CCM Signed-off-by: Imran Pochi --- .../cloudprovider/packet/README.md | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/cluster-autoscaler/cloudprovider/packet/README.md b/cluster-autoscaler/cloudprovider/packet/README.md index c82c19b28284..daf2fe073f60 100644 --- a/cluster-autoscaler/cloudprovider/packet/README.md +++ b/cluster-autoscaler/cloudprovider/packet/README.md @@ -79,6 +79,35 @@ affinity: - t1.small.x86 ``` +## CCM and Controller node labels + +### CCM +By default, autoscaler assumes that you have an older deprecated version of `packet-ccm` installed in your +cluster. If however, that is not the case and you've migrated to the new `cloud-provider-equinix-metal` CCM, +then this must be told to autoscaler. This can be done via setting an environment variable in the deployment: +``` +env: + - name: INSTALLED_CCM + value: cloud-provider-equinix-metal +``` +**NOTE**: As a prerequisite, ensure that all worker nodes in your cluster have the prefix `equinixmetal://` in +the Node spec `.spec.providerID`. If there are any existing worker nodes with prefix `packet://`, then drain +the node, remove the node and restart the kubelet on that worker node to re-register the node in the cluster, +this would ensure that `cloud-provider-equinix-metal` CCM sets the uuid with prefix `equinixmetal://` to the +field `.spec.ProviderID`. + +### Controller node labels + +Autoscaler assumes that control plane nodes in your cluster are identified by the label +`node-role.kubernetes.io/master`. If for some reason, this assumption is not true in your case, then set the +envirnment variable in the deployment: + +``` +env: + - name: PACKET_CONTROLLER_NODE_IDENTIFIER_LABEL + value: