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

feat+refactor: ability to skip some of the default checks #10324

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shellwhale
Copy link

Pull Request

What? (description)

Define the default checks in a map instead of directly returning them in a slice.
Add a new function PreBootSequenceChecksFiltered to allow users to purposefully skip some checks.

Why? (reasoning)

While using the Terraform Talos Provider and a custom CNI (Cilium), if installing Cilium via Terraform (Terraform Helm Provider) (not doing it via the machine config file) the talos_cluster_health data source fails.

See siderolabs/terraform-provider-talos#206

This PR would allow to modify the Talos Terraform Provider (which use this package) in a way that would provide users with the ability to skip some of the checks defined in default.go, one of those checks would be "kubelet to be healthy" in the case of siderolabs/terraform-provider-talos#206.

Perhaps better names for each of the individuals checks in this new map[string]ClusterCheck would be suitable, or maybe another solution all together.

@shellwhale shellwhale changed the title Ability to skip some of the default checks feat+refactor: Ability to skip some of the default checks Feb 10, 2025
@shellwhale shellwhale changed the title feat+refactor: Ability to skip some of the default checks feat+refactor: ability to skip some of the default checks Feb 10, 2025
@@ -13,57 +13,171 @@ import (
"github.com/siderolabs/talos/pkg/machinery/config/machine"
)

// PreBootSequenceChecks
const (
CheckEtcdHealthy = "etcd to be healthy"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as the package name is check, we can drop Check everywhere

@shellwhale
Copy link
Author

Hello, is there something that I can do to complete this merge?

@smira
Copy link
Member

smira commented Feb 24, 2025

Hello, is there something that I can do to complete this merge?

to address the review feedback?

@shellwhale shellwhale force-pushed the shellwhale/filterable-default-checks branch 9 times, most recently from 82fe09a to e4e8776 Compare February 25, 2025 06:14
Add a getCheck function and refactor checks to constants.
Add a new function PreBootSequenceChecksFiltered to allow users to purposefully skip some checks.

Signed-off-by: shellwhale <[email protected]>
@shellwhale shellwhale force-pushed the shellwhale/filterable-default-checks branch from e4e8776 to 5f75543 Compare February 25, 2025 06:17
@shellwhale
Copy link
Author

@smira done

@smira
Copy link
Member

smira commented Feb 25, 2025

@smira done

I don't see any changes, still everything is prefixed with Check, so it is check.Check*

@shellwhale
Copy link
Author

@smira done

I don't see any changes, still everything is prefixed with Check, so it is check.Check*

I cannot do that because of

	case CheckAllNodesMemorySizes // Cannot drop the check word
		return func(cluster ClusterInfo) conditions.Condition {
			return conditions.PollingCondition(CheckAllNodesMemorySizes, func(ctx context.Context) error {
				return AllNodesMemorySizes(ctx, cluster) // Because it would result in the same name as this
			}, 5*time.Minute, 5*time.Second)
		}

@smira
Copy link
Member

smira commented Feb 26, 2025

I cannot do that because of

probably these functions can be made private now? they should not be used outside of this package?

@shellwhale
Copy link
Author

I cannot do that because of

probably these functions can be made private now? they should not be used outside of this package?

Maybe but this is pkg, not internal, so that would be a breaking change, is that what we want?

@smira
Copy link
Member

smira commented Feb 26, 2025

I cannot do that because of

probably these functions can be made private now? they should not be used outside of this package?

Maybe but this is pkg, not internal, so that would be a breaking change, is that what we want?

Anyways it's a big change, so I'm ok with making it even bigger as long as it is consistent and bigger API

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.

2 participants