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.

Sign up for GitHub

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

Jump to bottom

Fix defaulting of Replicas spec #228

Merged
merged 2 commits into from
Jul 18, 2023
Merged

Fix defaulting of Replicas spec #228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30dce8b
Fix defaulting of Replicas spec
kajinamit Jul 12, 2023
da4c92d
Stop hard-coding container images in sample files
kajinamit Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/bases/cinder.openstack.org_cinderapis.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down
1 change: 1 addition & 0 deletions api/bases/cinder.openstack.org_cinderbackups.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down
4 changes: 4 additions & 0 deletions api/bases/cinder.openstack.org_cinders.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down Expand Up @@ -159,6 +160,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down Expand Up @@ -225,6 +227,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down Expand Up @@ -293,6 +296,7 @@ spec:
default: 1
format: int32
maximum: 1
minimum: 0
type: integer
resources:
properties:
Expand Down
1 change: 1 addition & 0 deletions api/bases/cinder.openstack.org_cinderschedulers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down
1 change: 1 addition & 0 deletions api/bases/cinder.openstack.org_cindervolumes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,7 @@ spec:
default: 1
format: int32
maximum: 1
minimum: 0
type: integer
resources:
properties:
Expand Down
5 changes: 3 additions & 2 deletions api/v1beta1/cinderapi_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ type CinderAPITemplate struct {

// +kubebuilder:validation:Optional
// +kubebuilder:default=1
// +kubebuilder:validation:Minimum=0
// Replicas - Cinder API Replicas
Replicas int32 `json:"replicas"`
Replicas *int32 `json:"replicas"`
Copy link
Contributor

Choose a reason for hiding this comment

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

As an old school C programmer in the distant past, I'm struggling to understand how this works. I gather the defaulting webhook will zero these fields, yielding the equivalent of a NULL pointer. Then the kubebuilder annotation will result in the replicas field being updated to point (because it's a pointer) to the default value, meaning the pointer is updated. Somehow kubebuilder knows how to set default values by reference AND by value (two different use cases).

Is my understanding anything close to correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I gather the defaulting webhook will zero these fields

I'm not sure if the defaulting webhook sets the pointers to the zero-value of nil or if it just ignores them completely and thus they would remain nil as they would presumably already be. But anyhow, then when they subsequently reach the kubebuilder logic, that logic indeed knows how to set the default value whether it's by reference or by value. So your understanding is correct.


// +kubebuilder:validation:Optional
// ExternalEndpoints, expose a VIP via MetalLB on the pre-created address pool
Expand Down Expand Up @@ -115,5 +116,5 @@ func init() {

// IsReady - returns true if service is ready to serve requests
func (instance CinderAPI) IsReady() bool {
return instance.Status.ReadyCount == instance.Spec.Replicas
return instance.Status.ReadyCount == *instance.Spec.Replicas
}
5 changes: 3 additions & 2 deletions api/v1beta1/cinderbackup_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ type CinderBackupTemplate struct {

// +kubebuilder:validation:Optional
// +kubebuilder:default=1
// +kubebuilder:validation:Minimum=0
// Replicas - Cinder Backup Replicas
Replicas int32 `json:"replicas"`
Replicas *int32 `json:"replicas"`
}

// CinderBackupSpec defines the desired state of CinderBackup
Expand Down Expand Up @@ -102,5 +103,5 @@ func init() {

// IsReady - returns true if service is ready to serve requests
func (instance CinderBackup) IsReady() bool {
return instance.Status.ReadyCount == instance.Spec.Replicas
return instance.Status.ReadyCount == *instance.Spec.Replicas
}
5 changes: 3 additions & 2 deletions api/v1beta1/cinderscheduler_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ type CinderSchedulerTemplate struct {

// +kubebuilder:validation:Optional
// +kubebuilder:default=1
// +kubebuilder:validation:Minimum=0
// Replicas - Cinder Scheduler Replicas
Replicas int32 `json:"replicas"`
Replicas *int32 `json:"replicas"`
}

// CinderSchedulerSpec defines the desired state of CinderScheduler
Expand Down Expand Up @@ -102,5 +103,5 @@ func init() {

// IsReady - returns true if service is ready to serve requests
func (instance CinderScheduler) IsReady() bool {
return instance.Status.ReadyCount == instance.Spec.Replicas
return instance.Status.ReadyCount == *instance.Spec.Replicas
}
5 changes: 3 additions & 2 deletions api/v1beta1/cindervolume_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ type CinderVolumeTemplate struct {

// +kubebuilder:validation:Optional
// +kubebuilder:default=1
// +kubebuilder:validation:Minimum=0
// +kubebuilder:validation:Maximum=1
// Replicas - Cinder Volume Replicas
Replicas int32 `json:"replicas"`
Replicas *int32 `json:"replicas"`
}

// CinderVolumeSpec defines the desired state of CinderVolume
Expand Down Expand Up @@ -103,5 +104,5 @@ func init() {

// IsReady - returns true if service is ready to serve requests
func (instance CinderVolume) IsReady() bool {
return instance.Status.ReadyCount == instance.Spec.Replicas
return instance.Status.ReadyCount == *instance.Spec.Replicas
}
20 changes: 20 additions & 0 deletions api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions config/crd/bases/cinder.openstack.org_cinderapis.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down
1 change: 1 addition & 0 deletions config/crd/bases/cinder.openstack.org_cinderbackups.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down
4 changes: 4 additions & 0 deletions config/crd/bases/cinder.openstack.org_cinders.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down Expand Up @@ -159,6 +160,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down Expand Up @@ -225,6 +227,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down Expand Up @@ -293,6 +296,7 @@ spec:
default: 1
format: int32
maximum: 1
minimum: 0
type: integer
resources:
properties:
Expand Down
1 change: 1 addition & 0 deletions config/crd/bases/cinder.openstack.org_cinderschedulers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,7 @@ spec:
replicas:
default: 1
format: int32
minimum: 0
type: integer
resources:
properties:
Expand Down
1 change: 1 addition & 0 deletions config/crd/bases/cinder.openstack.org_cindervolumes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,7 @@ spec:
default: 1
format: int32
maximum: 1
minimum: 0
type: integer
resources:
properties:
Expand Down
18 changes: 5 additions & 13 deletions config/samples/cinder_v1beta1_cinder.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,9 @@ spec:
databaseInstance: openstack
databaseUser: cinder
rabbitMqClusterName: rabbitmq
cinderAPI:
replicas: 1
containerImage: quay.io/podified-antelope-centos9/openstack-cinder-api:current-podified
cinderScheduler:
replicas: 1
containerImage: quay.io/podified-antelope-centos9/openstack-cinder-scheduler:current-podified
cinderBackup:
replicas: 1
containerImage: quay.io/podified-antelope-centos9/openstack-cinder-backup:current-podified
secret: cinder-secret
cinderAPI: {}
cinderScheduler: {}
cinderBackup: {}
cinderVolumes:
volume1:
containerImage: quay.io/podified-antelope-centos9/openstack-cinder-volume:current-podified
replicas: 1
volume1: {}
secret: cinder-secret
2 changes: 1 addition & 1 deletion controllers/cinder_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder
// Many OpenStack deployments don't use the cinder-backup service (it's optional),
// so there's no need to deploy it unless it's required.
var backupCondition *condition.Condition
if instance.Spec.CinderBackup.Replicas > 0 {
if *instance.Spec.CinderBackup.Replicas > 0 {
cinderBackup, op, err := r.backupDeploymentCreateOrUpdate(ctx, instance)
if err != nil {
instance.Status.Conditions.Set(condition.FalseCondition(
Expand Down
2 changes: 1 addition & 1 deletion controllers/cinderapi_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ func (r *CinderAPIReconciler) reconcileNormal(ctx context.Context, instance *cin
// verify if network attachment matches expectations
networkReady := false
networkAttachmentStatus := map[string][]string{}
if instance.Spec.Replicas > 0 {
if *instance.Spec.Replicas > 0 {
networkReady, networkAttachmentStatus, err = nad.VerifyNetworkStatusFromAnnotation(
ctx,
helper,
Expand Down
2 changes: 1 addition & 1 deletion controllers/cinderbackup_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ func (r *CinderBackupReconciler) reconcileNormal(ctx context.Context, instance *
// verify if network attachment matches expectations
networkReady := false
networkAttachmentStatus := map[string][]string{}
if instance.Spec.Replicas > 0 {
if *instance.Spec.Replicas > 0 {
networkReady, networkAttachmentStatus, err = nad.VerifyNetworkStatusFromAnnotation(
ctx,
helper,
Expand Down
2 changes: 1 addition & 1 deletion controllers/cinderscheduler_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ func (r *CinderSchedulerReconciler) reconcileNormal(ctx context.Context, instanc
// verify if network attachment matches expectations
networkReady := false
networkAttachmentStatus := map[string][]string{}
if instance.Spec.Replicas > 0 {
if *instance.Spec.Replicas > 0 {
networkReady, networkAttachmentStatus, err = nad.VerifyNetworkStatusFromAnnotation(
ctx,
helper,
Expand Down
2 changes: 1 addition & 1 deletion controllers/cindervolume_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ func (r *CinderVolumeReconciler) reconcileNormal(ctx context.Context, instance *
// verify if network attachment matches expectations
networkReady := false
networkAttachmentStatus := map[string][]string{}
if instance.Spec.Replicas > 0 {
if *instance.Spec.Replicas > 0 {
networkReady, networkAttachmentStatus, err = nad.VerifyNetworkStatusFromAnnotation(
ctx,
helper,
Expand Down
2 changes: 1 addition & 1 deletion pkg/cinderapi/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func Deployment(
Selector: &metav1.LabelSelector{
MatchLabels: labels,
},
Replicas: &instance.Spec.Replicas,
Replicas: instance.Spec.Replicas,
Copy link
Contributor

Choose a reason for hiding this comment

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

This part confuses me (even the original code). According to the StatefulSetSpec, the Replicas field is an int32 (not a pointer), so I don't know how we can mix pointers and pointer references.

Copy link
Contributor

Choose a reason for hiding this comment

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

For whatever reason, that documentation is not accurate. If you look at the code in an interactive IDE, the imported libraries for StatefulSet, Deployment, etc, have an *int32 as the data type for that field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for explaining this as a doc error. You've saved me from an eternity of staring into the abyss.

Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Annotations: annotations,
Expand Down
2 changes: 1 addition & 1 deletion pkg/cinderbackup/statefulset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func StatefulSet(
Selector: &metav1.LabelSelector{
MatchLabels: labels,
},
Replicas: &instance.Spec.Replicas,
Replicas: instance.Spec.Replicas,
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Annotations: annotations,
Expand Down
2 changes: 1 addition & 1 deletion pkg/cinderscheduler/statefulset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func StatefulSet(
Selector: &metav1.LabelSelector{
MatchLabels: labels,
},
Replicas: &instance.Spec.Replicas,
Replicas: instance.Spec.Replicas,
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Annotations: annotations,
Expand Down
2 changes: 1 addition & 1 deletion pkg/cindervolume/statefulset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func StatefulSet(
Selector: &metav1.LabelSelector{
MatchLabels: labels,
},
Replicas: &instance.Spec.Replicas,
Replicas: instance.Spec.Replicas,
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Annotations: annotations,
Expand Down
Loading