Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autoscaler multi architecture #728

Merged
merged 1 commit into from
Apr 16, 2023
Merged

Conversation

mysticaltech
Copy link
Collaborator

@mysticaltech mysticaltech commented Apr 16, 2023

Added architecture auto selection for the autoscaler. But the way it is now, it's the same architecture for all autoscaler nodepools, I do not see a way out of that constraint.

Also made other small tweaks, especially to the packer file to make it easier to edit, and also made the createkh script better.

FYI @aleksasiriski @ifeulner

@mysticaltech mysticaltech changed the base branch from master to staging April 16, 2023 06:39
@mysticaltech mysticaltech merged commit e45b7de into staging Apr 16, 2023
1 check passed
@mysticaltech mysticaltech deleted the autoscaler-multi-architecture branch April 16, 2023 06:41
@aleksasiriski
Copy link
Member

Hmm, is it possible to have 2 autoscalers on one cluster? I'll look into this when I get a chance

@ifeulner
Copy link
Contributor

if I understand this correctly kubernetes/autoscaler@6e94d1a it should be possible now to use a node group with a ca* instance and the implementation chooses the right image. So it should then work out of the box.

@mysticaltech
Copy link
Collaborator Author

@ifeulner @aleksasiriski Very interesting, if we use autoscaler node groups, then it should work. PR welcome folks!

@ifeulner
Copy link
Contributor

@ifeulner @aleksasiriski Very interesting, if we use autoscaler node groups, then it should work. PR welcome folks!

No PR needed, if that is implemented. We already support multiple node groups in the autoscaler setup.

@mysticaltech
Copy link
Collaborator Author

@ifeulner Have you seen the code for this very PR, no way this is going to work for multiple autoscaler nodepools of different types. It is designed for one type, based on checking out the server_type of the first autoscaler nodepool.

@ifeulner
Copy link
Contributor

ifeulner commented Apr 17, 2023

@mysticaltech Maybe I am wrong, but look here: https://github.com/kubernetes/autoscaler/blob/1009797f5585d7bf778072ba59fd12eb2b8ab83c/cluster-autoscaler/cloudprovider/hetzner/hetzner_node_group.go#L390

The updated findImage function is called within the context of a nodeGroup. So it should work from my understanding. Will try later.

func createServer(n *hetznerNodeGroup) error {
	serverType, err := n.manager.cachedServerType.getServerType(n.instanceType)
	if err != nil {
		return err
	}

	image, err := findImage(n, serverType)
	if err != nil {
		return err
	}

Comment from findImage:

// findImage searches for an image ID corresponding to the supplied
// HCLOUD_IMAGE env variable. This value can either be an image ID itself (an
// int), a name (e.g. "ubuntu-20.04"), or a label selector associated with an
// image snapshot. In the latter case it will use the most recent snapshot.
// It also verifies that the returned image has a compatible architecture with
// server.

@mysticaltech
Copy link
Collaborator Author

mysticaltech commented Apr 17, 2023

Very interesting, we may need to delete lines. Maybe only the selector is needed indeed.

@ifeulner
Copy link
Contributor

Will check, need to find out if these patches are already deployed officially...

@ifeulner
Copy link
Contributor

ifeulner commented Apr 17, 2023

@mysticaltech so now I see our misunderstanding. Our implementation here

first_nodepool_snapshot_id = length(var.autoscaler_nodepools) == 0 ? "" : (
is blocking now the freshly added multi-arch support by the provider. We need to refer to the image name here (which must be the same for ARM and x86), and let the autoscaler-provider choose the right image.

So there is a PR needed for our code ;)

@aleksasiriski
Copy link
Member

Couldn't we just use the same named label selector? Like how it is currenty?

@ifeulner
Copy link
Contributor

ifeulner commented Apr 17, 2023

should work also according to the documentation above, yes.

Will try and provide a PR if it is working.

@ifeulner
Copy link
Contributor

it works with the label-selector but the Hetzner cluster-autoscaler changes mentioned above are not yet released.
(latest version currently cluster-autoscaler:v1.26.2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants