Skip to content

Conversation

@bosiakov
Copy link
Contributor

Implements #34

  • gofmt
  • Runner as a separate structure
  • Added ResourceRequirements
  • Added Tolerations
  • Added NodeSelector
  • Added PodMetadata
  • Added PriorityClassName
  • Added runAsUser

@bosiakov bosiakov marked this pull request as draft November 16, 2024 12:43
@bosiakov bosiakov marked this pull request as ready for review November 16, 2024 12:55
}

// RunnerConfig defines the configuration of runner image
type RunnerConfig struct {
Copy link
Member

@akyriako akyriako Nov 19, 2024

Choose a reason for hiding this comment

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

I think it a long time, that this should be the way to go; moving all configuration for runners in a separate struct. But I think it should be introduced as a new api version for this Kind (e.g v1alpha2) and provide a convertor from v1alpha1 to v1alpha2; otherwise it breaks backwards compatibility and existing SleepCycles from previous versions. If you try to run it on a cluster where existing SleepCycles exist, it will break:

image

A small example here: https://suedbroecker.net/2022/03/24/add-a-new-api-version-to-an-existing-operator/ (although the way it describes the changes that need to take place on the controller are a bit debatable and need more thorough analysis)

P.S: the project is not yet upgraded to kubebuilder v4 (still on v3; issue #29 which I am actively working on), so if the latest version of kubebuilder is installed on the development environment any attempt to add a new api will fail.

// +kubebuilder:validation:ExclusiveMaximum=false
FailedJobsHistoryLimit int32 `json:"failedJobsHistoryLimit,omitempty"`

Runner RunnerConfig `json:"runner,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Why not *RunnerConfig?

return &v1.ResourceRequirements{
Limits: v1.ResourceList{
"cpu": resource.MustParse("10m"),
"memory": resource.MustParse("128mb"),
Copy link
Member

@akyriako akyriako Nov 19, 2024

Choose a reason for hiding this comment

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

That is not parsing, lines 132 and 136 need to change to 128Mi

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