-
Notifications
You must be signed in to change notification settings - Fork 392
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
[OCPCLOUD-1209] Run KubeletConfig FeatureGate sync during bootstrap #2668
Changes from all commits
118ad16
64cdcdf
0e51573
5f95629
36b4168
0c55518
1b4429b
46b6ace
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,9 @@ import ( | |
apicfgv1 "github.com/openshift/api/config/v1" | ||
apioperatorsv1alpha1 "github.com/openshift/api/operator/v1alpha1" | ||
mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" | ||
ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" | ||
containerruntimeconfig "github.com/openshift/machine-config-operator/pkg/controller/container-runtime-config" | ||
kubeletconfig "github.com/openshift/machine-config-operator/pkg/controller/kubelet-config" | ||
"github.com/openshift/machine-config-operator/pkg/controller/render" | ||
"github.com/openshift/machine-config-operator/pkg/controller/template" | ||
) | ||
|
@@ -46,6 +48,7 @@ func New(templatesDir, manifestDir, pullSecretFile string) *Bootstrap { | |
|
||
// Run runs boostrap for Machine Config Controller | ||
// It writes all the assets to destDir | ||
// nolint:gocyclo | ||
func (b *Bootstrap) Run(destDir string) error { | ||
infos, err := ioutil.ReadDir(b.manifestDir) | ||
if err != nil { | ||
|
@@ -70,6 +73,7 @@ func (b *Bootstrap) Run(destDir string) error { | |
decoder := codecFactory.UniversalDecoder(mcfgv1.GroupVersion, apioperatorsv1alpha1.GroupVersion, apicfgv1.GroupVersion) | ||
|
||
var cconfig *mcfgv1.ControllerConfig | ||
var featureGate *apicfgv1.FeatureGate | ||
var pools []*mcfgv1.MachineConfigPool | ||
var configs []*mcfgv1.MachineConfig | ||
var icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy | ||
|
@@ -112,6 +116,10 @@ func (b *Bootstrap) Run(destDir string) error { | |
icspRules = append(icspRules, obj) | ||
case *apicfgv1.Image: | ||
imgCfg = obj | ||
case *apicfgv1.FeatureGate: | ||
if obj.GetName() == ctrlcommon.ClusterFeatureInstanceName { | ||
featureGate = obj | ||
} | ||
default: | ||
glog.Infof("skipping %q [%d] manifest because of unhandled %T", file.Name(), idx+1, obji) | ||
} | ||
|
@@ -121,7 +129,7 @@ func (b *Bootstrap) Run(destDir string) error { | |
if cconfig == nil { | ||
return fmt.Errorf("error: no controllerconfig found in dir: %q", destDir) | ||
} | ||
iconfigs, err := template.RunBootstrap(b.templatesDir, cconfig, psraw) | ||
iconfigs, err := template.RunBootstrap(b.templatesDir, cconfig, psraw, featureGate) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -133,6 +141,14 @@ func (b *Bootstrap) Run(destDir string) error { | |
} | ||
configs = append(configs, rconfigs...) | ||
|
||
if featureGate != nil { | ||
kConfigs, err := kubeletconfig.RunFeatureGateBootstrap(b.templatesDir, featureGate, cconfig, pools) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok so I want to wrap my head around this scenario so I understand this properly. In case of no user-provided featureGate (the default scenario), this Bootstrap portion does not run, which is normal, so there is no featuregate machineconfig being created. In the sync loop of the KubeletConfigController, we fetch in-cluster feature objects if they exist (can this be nil?), and pass them to generateFeatureMap to parse. I guess my question here is, can that feature object being fetched by Hopefully that question made a bit of sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok Joel helped me understand this a bit more. The default generation and "user provided" generation are different. There is always a default set of featuregates that's provided as part of the base kubelet MC |
||
if err != nil { | ||
return err | ||
} | ||
configs = append(configs, kConfigs...) | ||
} | ||
|
||
fpools, gconfigs, err := render.RunBootstrap(pools, configs, cconfig) | ||
if err != nil { | ||
return err | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -330,14 +330,43 @@ func (ctrl *Controller) handleFeatureErr(err error, key interface{}) { | |
ctrl.featureQueue.AddAfter(key, 1*time.Minute) | ||
} | ||
|
||
func (ctrl *Controller) generateOriginalKubeletConfig(role string, featureGate *configv1.FeatureGate) (*ign3types.File, error) { | ||
cc, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName) | ||
// generateOriginalKubeletConfigWithFeatureGates generates a KubeletConfig and ensure the correct feature gates are set | ||
// based on the given FeatureGate. | ||
func generateOriginalKubeletConfigWithFeatureGates(cc *mcfgv1.ControllerConfig, templatesDir, role string, features *configv1.FeatureGate) (*kubeletconfigv1beta1.KubeletConfiguration, error) { | ||
originalKubeletIgn, err := generateOriginalKubeletConfigIgn(cc, templatesDir, role, features) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not get ControllerConfig %v", err) | ||
return nil, fmt.Errorf("could not generate the original Kubelet config ignition: %v", err) | ||
} | ||
if originalKubeletIgn.Contents.Source == nil { | ||
return nil, fmt.Errorf("the original Kubelet source string is empty: %v", err) | ||
} | ||
dataURL, err := dataurl.DecodeString(*originalKubeletIgn.Contents.Source) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not decode the original Kubelet source string: %v", err) | ||
} | ||
originalKubeConfig, err := decodeKubeletConfig(dataURL.Data) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not deserialize the Kubelet source: %v", err) | ||
} | ||
|
||
featureGates, err := generateFeatureMap(features, openshiftOnlyFeatureGates...) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not generate features map: %v", err) | ||
} | ||
|
||
// Merge in Feature Gates. | ||
// If they are the same, this will be a no-op | ||
if err := mergo.Merge(&originalKubeConfig.FeatureGates, featureGates, mergo.WithOverride); err != nil { | ||
return nil, fmt.Errorf("could not merge feature gates: %v", err) | ||
} | ||
|
||
return originalKubeConfig, nil | ||
} | ||
|
||
func generateOriginalKubeletConfigIgn(cc *mcfgv1.ControllerConfig, templatesDir, role string, featureGate *configv1.FeatureGate) (*ign3types.File, error) { | ||
// Render the default templates | ||
rc := &mtmpl.RenderConfig{ControllerConfigSpec: &cc.Spec, FeatureGate: featureGate} | ||
generatedConfigs, err := mtmpl.GenerateMachineConfigsForRole(rc, role, ctrl.templatesDir) | ||
generatedConfigs, err := mtmpl.GenerateMachineConfigsForRole(rc, role, templatesDir) | ||
if err != nil { | ||
return nil, fmt.Errorf("GenerateMachineConfigsforRole failed with error %s", err) | ||
} | ||
|
@@ -478,12 +507,6 @@ func (ctrl *Controller) syncKubeletConfig(key string) error { | |
err := fmt.Errorf("could not fetch FeatureGates: %v", err) | ||
return ctrl.syncStatusOnly(cfg, err) | ||
} | ||
featureGates, err := generateFeatureMap(features) | ||
if err != nil { | ||
err := fmt.Errorf("could not generate FeatureMap: %v", err) | ||
glog.V(2).Infof("%v", err) | ||
return ctrl.syncStatusOnly(cfg, err) | ||
} | ||
|
||
for _, pool := range mcpPools { | ||
if pool.Spec.Configuration.Name == "" { | ||
|
@@ -512,20 +535,14 @@ func (ctrl *Controller) syncKubeletConfig(key string) error { | |
userDefinedSystemReserved := make(map[string]string, 2) | ||
|
||
// Generate the original KubeletConfig | ||
originalKubeletIgn, err := ctrl.generateOriginalKubeletConfig(role, features) | ||
if err != nil { | ||
return ctrl.syncStatusOnly(cfg, err, "could not generate the original Kubelet config: %v", err) | ||
} | ||
if originalKubeletIgn.Contents.Source == nil { | ||
return ctrl.syncStatusOnly(cfg, err, "the original Kubelet source string is empty: %v", err) | ||
} | ||
dataURL, err := dataurl.DecodeString(*originalKubeletIgn.Contents.Source) | ||
cc, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName) | ||
if err != nil { | ||
return ctrl.syncStatusOnly(cfg, err, "could not decode the original Kubelet source string: %v", err) | ||
return fmt.Errorf("could not get ControllerConfig %v", err) | ||
} | ||
originalKubeConfig, err := decodeKubeletConfig(dataURL.Data) | ||
|
||
originalKubeConfig, err := generateOriginalKubeletConfigWithFeatureGates(cc, ctrl.templatesDir, role, features) | ||
if err != nil { | ||
return ctrl.syncStatusOnly(cfg, err, "could not deserialize the Kubelet source: %v", err) | ||
return ctrl.syncStatusOnly(cfg, err, "could not get original kubelet config: %v", err) | ||
} | ||
|
||
// Get the default API Server Security Profile | ||
|
@@ -561,29 +578,21 @@ func (ctrl *Controller) syncKubeletConfig(key string) error { | |
delete(specKubeletConfig.SystemReserved, "cpu") | ||
} | ||
|
||
// FeatureGates must be set from the FeatureGate. | ||
// Remove them here to prevent the specKubeletConfig merge overwriting them. | ||
specKubeletConfig.FeatureGates = nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where does the FeatureGates get injected in for this object? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 543 uses a new kubelet generator function as part of the refactor, which is shared between this and the feature gate handler, which handles the feature gate injection
If you look at 1b4429b explicitly it's a bit easier to see what the change here is I could expand this comment to make this more clear
|
||
|
||
// Merge the Old and New | ||
err = mergo.Merge(originalKubeConfig, specKubeletConfig, mergo.WithOverride) | ||
if err != nil { | ||
return ctrl.syncStatusOnly(cfg, err, "could not merge original config and new config: %v", err) | ||
} | ||
// Merge in Feature Gates | ||
err = mergo.Merge(&originalKubeConfig.FeatureGates, featureGates, mergo.WithOverride) | ||
if err != nil { | ||
return ctrl.syncStatusOnly(cfg, err, "could not merge FeatureGates: %v", err) | ||
} | ||
// Encode the new config into raw JSON | ||
cfgJSON, err := EncodeKubeletConfig(originalKubeConfig, kubeletconfigv1beta1.SchemeGroupVersion) | ||
if err != nil { | ||
return ctrl.syncStatusOnly(cfg, err, "could not encode JSON: %v", err) | ||
} | ||
kubeletIgnition = createNewKubeletIgnition(cfgJSON) | ||
} else { | ||
// Encode the new config into raw JSON | ||
cfgJSON, err := EncodeKubeletConfig(originalKubeConfig, kubeletconfigv1beta1.SchemeGroupVersion) | ||
if err != nil { | ||
return ctrl.syncStatusOnly(cfg, err, "could not encode JSON: %v", err) | ||
} | ||
kubeletIgnition = createNewKubeletIgnition(cfgJSON) | ||
} | ||
|
||
// Encode the new config into an Ignition File | ||
kubeletIgnition, err = kubeletConfigToIgnFile(originalKubeConfig) | ||
if err != nil { | ||
return ctrl.syncStatusOnly(cfg, err, "could not encode JSON: %v", err) | ||
} | ||
|
||
if isNotFound { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,18 +8,16 @@ import ( | |
|
||
"github.com/clarketm/json" | ||
"github.com/golang/glog" | ||
"github.com/imdario/mergo" | ||
osev1 "github.com/openshift/api/config/v1" | ||
"github.com/openshift/library-go/pkg/cloudprovider" | ||
"github.com/vincent-petithory/dataurl" | ||
"k8s.io/apimachinery/pkg/api/errors" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/labels" | ||
utilruntime "k8s.io/apimachinery/pkg/util/runtime" | ||
"k8s.io/client-go/tools/cache" | ||
"k8s.io/client-go/util/retry" | ||
kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1" | ||
|
||
mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" | ||
ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" | ||
"github.com/openshift/machine-config-operator/pkg/version" | ||
) | ||
|
@@ -69,9 +67,10 @@ func (ctrl *Controller) syncFeatureHandler(key string) error { | |
} else if err != nil { | ||
return err | ||
} | ||
featureGates, err := generateFeatureMap(features, openshiftOnlyFeatureGates...) | ||
|
||
cc, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName) | ||
if err != nil { | ||
return err | ||
return fmt.Errorf("could not get ControllerConfig %v", err) | ||
} | ||
|
||
// Find all MachineConfigPools | ||
|
@@ -100,43 +99,15 @@ func (ctrl *Controller) syncFeatureHandler(key string) error { | |
return err | ||
} | ||
} | ||
// Generate the original KubeletConfig | ||
originalKubeletIgn, err := ctrl.generateOriginalKubeletConfig(role, nil) | ||
if err != nil { | ||
return err | ||
} | ||
if originalKubeletIgn.Contents.Source == nil { | ||
return fmt.Errorf("could not find original Kubelet config to decode") | ||
} | ||
dataURL, err := dataurl.DecodeString(*originalKubeletIgn.Contents.Source) | ||
if err != nil { | ||
return err | ||
} | ||
originalKubeConfig, err := decodeKubeletConfig(dataURL.Data) | ||
|
||
rawCfgIgn, err := generateKubeConfigIgnFromFeatures(cc, ctrl.templatesDir, role, features) | ||
if err != nil { | ||
return err | ||
} | ||
// Check to see if FeatureGates are equal | ||
if reflect.DeepEqual(originalKubeConfig.FeatureGates, *featureGates) { | ||
if rawCfgIgn == nil { | ||
continue | ||
} | ||
// Merge in Feature Gates | ||
err = mergo.Merge(&originalKubeConfig.FeatureGates, featureGates, mergo.WithOverride) | ||
if err != nil { | ||
return err | ||
} | ||
// Encode the new config into raw JSON | ||
cfgJSON, err := EncodeKubeletConfig(originalKubeConfig, kubeletconfigv1beta1.SchemeGroupVersion) | ||
if err != nil { | ||
return err | ||
} | ||
tempIgnConfig := ctrlcommon.NewIgnConfig() | ||
cfgIgn := createNewKubeletIgnition(cfgJSON) | ||
tempIgnConfig.Storage.Files = append(tempIgnConfig.Storage.Files, *cfgIgn) | ||
rawCfgIgn, err := json.Marshal(tempIgnConfig) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
mc.Spec.Config.Raw = rawCfgIgn | ||
mc.ObjectMeta.Annotations = map[string]string{ | ||
ctrlcommon.GeneratedByControllerVersionAnnotationKey: version.Hash, | ||
|
@@ -231,3 +202,70 @@ func generateFeatureMap(features *osev1.FeatureGate, exclusions ...string) (*map | |
} | ||
return &rv, nil | ||
} | ||
|
||
func generateKubeConfigIgnFromFeatures(cc *mcfgv1.ControllerConfig, templatesDir, role string, features *osev1.FeatureGate) ([]byte, error) { | ||
originalKubeConfig, err := generateOriginalKubeletConfigWithFeatureGates(cc, templatesDir, role, features) | ||
if err != nil { | ||
return nil, err | ||
} | ||
defaultFeatures, err := generateFeatureMap(createNewDefaultFeatureGate(), openshiftOnlyFeatureGates...) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// Check to see if configured FeatureGates are equivalent to the Default FeatureSet. | ||
if reflect.DeepEqual(originalKubeConfig.FeatureGates, *defaultFeatures) { | ||
// When there is no difference, this isn't an error, but no machine config should be created | ||
return nil, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this will work. Upon an upgrade, all MCs should be updated with at least the new controller version. If the featuregate doesn't change, we still have to return the same MC such that the sync funtion above doesn't hit the
which will not update the
which then in turn causes the MCO to fail because of a rendered version mismatch for the controller "(i.e. MCC sees that a controller-created config never got updated). So we should still return the same config so the above sync happens. Have you tried installing (from existing releases), adding a feature gate and then upgrading to this PR? That should be a good check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic here hasn't changed as far as I understand it, the idea of this function was just to extract the logic that was already in place, but not change the logic. Before, there was this same check that caused a I think the reason this works is because we are comparing the default feature gates (ie if you didn't have a Also note that if you have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see, defaultFeatures is static, ok that makes sense I think. When we say "can't upgrade" we mean we cannot upgrade y-stream correct? Based on the CI job it seems featuregates are blocked from upgrading at the y-stream level but not at the z-stream level, since it makes use of upgradeable=false. Just wanted to check that understanding |
||
} | ||
|
||
// Encode the new config into raw JSON | ||
cfgIgn, err := kubeletConfigToIgnFile(originalKubeConfig) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
tempIgnConfig := ctrlcommon.NewIgnConfig() | ||
tempIgnConfig.Storage.Files = append(tempIgnConfig.Storage.Files, *cfgIgn) | ||
rawCfgIgn, err := json.Marshal(tempIgnConfig) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return rawCfgIgn, nil | ||
} | ||
|
||
func RunFeatureGateBootstrap(templateDir string, features *osev1.FeatureGate, controllerConfig *mcfgv1.ControllerConfig, mcpPools []*mcfgv1.MachineConfigPool) ([]*mcfgv1.MachineConfig, error) { | ||
machineConfigs := []*mcfgv1.MachineConfig{} | ||
|
||
for _, pool := range mcpPools { | ||
role := pool.Name | ||
rawCfgIgn, err := generateKubeConfigIgnFromFeatures(controllerConfig, templateDir, role, features) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if rawCfgIgn == nil { | ||
continue | ||
} | ||
|
||
// Get MachineConfig | ||
managedKey, err := getManagedFeaturesKey(pool, nil) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
ignConfig := ctrlcommon.NewIgnConfig() | ||
mc, err := ctrlcommon.MachineConfigFromIgnConfig(role, managedKey, ignConfig) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
mc.Spec.Config.Raw = rawCfgIgn | ||
mc.ObjectMeta.Annotations = map[string]string{ | ||
ctrlcommon.GeneratedByControllerVersionAnnotationKey: version.Hash, | ||
} | ||
|
||
machineConfigs = append(machineConfigs, mc) | ||
} | ||
|
||
return machineConfigs, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm interpreting this correctly, since you are not storing this as an array, the expected behaviour is that the user only applies 1 config. On the off chance they supply multiple, it will not error but instead have the higher alphanumeric take priority?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, so the expectation is that a customer should only create one, whichever is read last will win, not sure if you want to have logic in here to account for that or not? But I'd expect a customer to only supply one.
Also note, it has to have a particular name,
cluster
, so that's why we expect a singleton here