Skip to content

[release-4.1] Bug 1706082: Add Spec.Configuration to MachineConfigPool, render controller writes it #856

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

Merged
openshift-merge-robot merged 3 commits into from
Jun 25, 2019
Merged

[release-4.1] Bug 1706082: Add Spec.Configuration to MachineConfigPool, render controller writes it #856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a400aae
tests: Factor out helper to wait for rendered MC
cgwalters May 16, 2019
cfa7f6c
test/e2e: More refactoring
cgwalters May 16, 2019
7cdc5b7
Add Spec.Configuration to MachineConfigPool, render controller writes it
cgwalters May 16, 2019
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
3 changes: 3 additions & 0 deletions pkg/apis/machineconfiguration.openshift.io/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,9 @@ type MachineConfigPoolSpec struct {
// MaxUnavailable specifies the percentage or constant number of machines that can be updating at any given time.
// default is 1.
MaxUnavailable *intstr.IntOrString `json:"maxUnavailable"`

// The targeted MachineConfig object for the machine config pool.
Configuration MachineConfigPoolStatusConfiguration `json:"configuration"`
}

// MachineConfigPoolStatus is the status for MachineConfigPool resource.
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/machineconfiguration.openshift.io/v1/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 pkg/controller/container-runtime-config/container_runtime_config_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ func newMachineConfigPool(name string, labels map[string]string, selector *metav
ObjectMeta: metav1.ObjectMeta{Name: name, Labels: labels, UID: types.UID(utilrand.String(5))},
Spec: mcfgv1.MachineConfigPoolSpec{
MachineConfigSelector: selector,
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
},
Status: mcfgv1.MachineConfigPoolStatus{
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/kubelet-config/kubelet_config_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ func newMachineConfigPool(name string, labels map[string]string, selector *metav
ObjectMeta: metav1.ObjectMeta{Name: name, Labels: labels, UID: types.UID(utilrand.String(5))},
Spec: mcfgv1.MachineConfigPoolSpec{
MachineConfigSelector: selector,
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
},
Status: mcfgv1.MachineConfigPoolStatus{
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
Expand Down
6 changes: 3 additions & 3 deletions pkg/controller/node/node_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
return err
}

if machineconfigpool.Status.Configuration.Name == "" {
if machineconfigpool.Spec.Configuration.Name == "" {
// Previously we spammed the logs about empty pools.
// Let's just pause for a bit here to let the renderer
// initialize them.
Expand Down Expand Up @@ -475,7 +475,7 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {

candidates := getCandidateMachines(pool, nodes, maxunavail)
for _, node := range candidates {
if err := ctrl.setDesiredMachineConfigAnnotation(node.Name, pool.Status.Configuration.Name); err != nil {
if err := ctrl.setDesiredMachineConfigAnnotation(node.Name, pool.Spec.Configuration.Name); err != nil {
return err
}
}
Expand Down Expand Up @@ -517,7 +517,7 @@ func (ctrl *Controller) setDesiredMachineConfigAnnotation(nodeName, currentConfi
}

func getCandidateMachines(pool *mcfgv1.MachineConfigPool, nodesInPool []*corev1.Node, maxUnavailable int) []*corev1.Node {
targetConfig := pool.Status.Configuration.Name
targetConfig := pool.Spec.Configuration.Name

unavail := getUnavailableMachines(nodesInPool)
// If we're at capacity, there's nothing to do.
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/node/node_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func newMachineConfigPool(name string, selector *metav1.LabelSelector, maxUnavai
Spec: mcfgv1.MachineConfigPoolSpec{
NodeSelector: selector,
MaxUnavailable: maxUnavail,
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
},
Status: mcfgv1.MachineConfigPoolStatus{
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
Expand Down Expand Up @@ -502,7 +503,7 @@ func TestGetCandidateMachines(t *testing.T) {
for idx, test := range tests {
t.Run(fmt.Sprintf("case#%d", idx), func(t *testing.T) {
pool := &mcfgv1.MachineConfigPool{
Status: mcfgv1.MachineConfigPoolStatus{
Spec: mcfgv1.MachineConfigPoolSpec{
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: "v1"}},
},
}
Expand Down
24 changes: 13 additions & 11 deletions pkg/controller/node/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,12 @@ func (ctrl *Controller) syncStatusOnly(pool *mcfgv1.MachineConfigPool) error {
}

func calculateStatus(pool *mcfgv1.MachineConfigPool, nodes []*corev1.Node) mcfgv1.MachineConfigPoolStatus {
previousUpdated := mcfgv1.IsMachineConfigPoolConditionTrue(pool.Status.Conditions, mcfgv1.MachineConfigPoolUpdated)

machineCount := int32(len(nodes))

updatedMachines := getUpdatedMachines(pool.Status.Configuration.Name, nodes)
updatedMachines := getUpdatedMachines(pool.Spec.Configuration.Name, nodes)
updatedMachineCount := int32(len(updatedMachines))

readyMachines := getReadyMachines(pool.Status.Configuration.Name, nodes)
readyMachines := getReadyMachines(pool.Spec.Configuration.Name, nodes)
readyMachineCount := int32(len(readyMachines))

unavailableMachines := getUnavailableMachines(nodes)
Expand Down Expand Up @@ -73,22 +71,26 @@ func calculateStatus(pool *mcfgv1.MachineConfigPool, nodes []*corev1.Node) mcfgv
status.Conditions = append(status.Conditions, conditions[i])
}

if updatedMachineCount == machineCount &&
allUpdated := updatedMachineCount == machineCount &&
readyMachineCount == machineCount &&
unavailableMachineCount == 0 {
unavailableMachineCount == 0

if allUpdated {
//TODO: update api to only have one condition regarding status of update.
updatedMsg := fmt.Sprintf("All nodes are updated with %s", pool.Status.Configuration.Name)
updatedMsg := fmt.Sprintf("All nodes are updated with %s", pool.Spec.Configuration.Name)
supdated := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdated, corev1.ConditionTrue, updatedMsg, "")
mcfgv1.SetMachineConfigPoolCondition(&status, *supdated)
if !previousUpdated {
glog.Infof("Pool %s: %s", pool.Name, updatedMsg)
}

supdating := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdating, corev1.ConditionFalse, "", "")
mcfgv1.SetMachineConfigPoolCondition(&status, *supdating)
if status.Configuration.Name != pool.Spec.Configuration.Name {
glog.Infof("Pool %s: %s", pool.Name, updatedMsg)
status.Configuration = pool.Spec.Configuration
}
} else {
supdated := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdated, corev1.ConditionFalse, "", "")
mcfgv1.SetMachineConfigPoolCondition(&status, *supdated)
supdating := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdating, corev1.ConditionTrue, fmt.Sprintf("All nodes are updating to %s", pool.Status.Configuration.Name), "")
supdating := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdating, corev1.ConditionTrue, fmt.Sprintf("All nodes are updating to %s", pool.Spec.Configuration.Name), "")
mcfgv1.SetMachineConfigPoolCondition(&status, *supdating)
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/node/status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ func TestCalculateStatus(t *testing.T) {
for idx, test := range tests {
t.Run(fmt.Sprintf("case#%d", idx), func(t *testing.T) {
pool := &mcfgv1.MachineConfigPool{
Status: mcfgv1.MachineConfigPoolStatus{
Spec: mcfgv1.MachineConfigPoolSpec{
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: test.currentConfig}},
},
}
Expand Down
13 changes: 8 additions & 5 deletions pkg/controller/render/render_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,18 +500,20 @@ func (ctrl *Controller) syncGeneratedMachineConfig(pool *mcfgv1.MachineConfigPoo
return err
}

if pool.Status.Configuration.Name == generated.Name {
if pool.Spec.Configuration.Name == generated.Name {
_, _, err = resourceapply.ApplyMachineConfig(ctrl.client.MachineconfigurationV1(), generated)
return err
}

glog.V(2).Infof("Pool %s is now targeting: %s", pool.Name, generated.Name)
pool.Status.Configuration.Name = generated.Name
pool.Status.Configuration.Source = source
_, err = ctrl.client.MachineconfigurationV1().MachineConfigPools().UpdateStatus(pool)
newPool := pool.DeepCopy()
newPool.Spec.Configuration.Name = generated.Name
newPool.Spec.Configuration.Source = source
// TODO(walters) Use subresource or JSON patch, but the latter isn't supported by the unit test mocks
pool, err = ctrl.client.MachineconfigurationV1().MachineConfigPools().Update(newPool)
if err != nil {
return err
}
glog.V(2).Infof("Pool %s: now targeting: %s", pool.Name, generated.Name)

if err := ctrl.garbageCollectRenderedConfigs(pool); err != nil {
return err
Expand Down Expand Up @@ -565,6 +567,7 @@ func RunBootstrap(pools []*mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineCo
return nil, nil, err
}

pool.Spec.Configuration.Name = generated.Name
pool.Status.Configuration.Name = generated.Name
opools = append(opools, pool)
oconfigs = append(oconfigs, generated)
Expand Down
7 changes: 7 additions & 0 deletions pkg/controller/render/render_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ func newMachineConfigPool(name string, selector *metav1.LabelSelector, currentMa
ObjectMeta: metav1.ObjectMeta{Name: name, UID: types.UID(utilrand.String(5))},
Spec: mcfgv1.MachineConfigPoolSpec{
MachineConfigSelector: selector,
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
},
Status: mcfgv1.MachineConfigPoolStatus{
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
Expand Down Expand Up @@ -235,6 +236,10 @@ func (f *fixture) expectUpdateMachineConfigAction(config *mcfgv1.MachineConfig)
f.actions = append(f.actions, core.NewRootUpdateAction(schema.GroupVersionResource{Resource: "machineconfigs"}, config))
}

func (f *fixture) expectUpdateMachineConfigPoolSpec(pool *mcfgv1.MachineConfigPool) {
f.actions = append(f.actions, core.NewRootUpdateSubresourceAction(schema.GroupVersionResource{Resource: "machineconfigpools"}, "spec", pool))
}

func (f *fixture) expectUpdateMachineConfigPoolStatus(pool *mcfgv1.MachineConfigPool) {
f.actions = append(f.actions, core.NewRootUpdateSubresourceAction(schema.GroupVersionResource{Resource: "machineconfigpools"}, "status", pool))
}
Expand Down Expand Up @@ -339,6 +344,7 @@ func TestUpdatesGeneratedMachineConfig(t *testing.T) {
t.Fatal(err)
}
gmc.Spec.OSImageURL = "why-did-you-change-it"
mcp.Spec.Configuration.Name = gmc.Name
mcp.Status.Configuration.Name = gmc.Name

f.ccLister = append(f.ccLister, cc)
Expand Down Expand Up @@ -401,6 +407,7 @@ func TestDoNothing(t *testing.T) {
if err != nil {
t.Fatal(err)
}
mcp.Spec.Configuration.Name = gmc.Name
mcp.Status.Configuration.Name = gmc.Name

f.ccLister = append(f.ccLister, cc)
Expand Down
5 changes: 4 additions & 1 deletion pkg/operator/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,11 @@ func (optr *Operator) allMachineConfigPoolStatus() (map[string]string, error) {
// isMachineConfigPoolConfigurationValid returns nil error when the configuration of a `pool` is created by the controller at version `version`.
func isMachineConfigPoolConfigurationValid(pool *mcfgv1.MachineConfigPool, version string, machineConfigGetter func(string) (*mcfgv1.MachineConfig, error)) error {
// both .status.configuration.name and .status.configuration.source must be set.
if len(pool.Spec.Configuration.Name) == 0 {
return fmt.Errorf("configuration spec for pool %s is empty", pool.GetName())
}
if len(pool.Status.Configuration.Name) == 0 {
return fmt.Errorf("configuration for pool %s is empty", pool.GetName())
return fmt.Errorf("configuration status for pool %s is empty", pool.GetName())
}
if len(pool.Status.Configuration.Source) == 0 {
return fmt.Errorf("list of MachineConfigs that were used to generate configuration for pool %s is empty", pool.GetName())
Expand Down
5 changes: 4 additions & 1 deletion pkg/operator/status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestIsMachineConfigPoolConfigurationValid(t *testing.T) {

err error
}{{
err: errors.New("configuration for pool dummy-pool is empty"),
err: errors.New("configuration spec for pool dummy-pool is empty"),
}, {
generated: "g",
err: errors.New("list of MachineConfigs that were used to generate configuration for pool dummy-pool is empty"),
Expand Down Expand Up @@ -116,6 +116,9 @@ func TestIsMachineConfigPoolConfigurationValid(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "dummy-pool",
},
Spec: mcfgv1.MachineConfigPoolSpec{
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: test.generated}, Source: source},
},
Status: mcfgv1.MachineConfigPoolStatus{
Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: test.generated}, Source: source},
},
Expand Down
Loading