WIP: Add machineconfiguration API group by LorbusChris · Pull Request #620 · openshift/api

LorbusChris · 2020-04-06T14:28:58Z

This moves over the machineconfiguration API from the
github.com/openshift/machine-config-operator repo,
and adds swagger doc generation for it.

TODO:

Re-add ControllerConfig
Exclude RawExtension from OpenAPI Validation generation
Address review here

/cc @runcom

openshift-ci-robot · 2020-04-06T14:29:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LorbusChris
To complete the pull request process, please assign sjenning
You can assign the PR to them by writing /assign @sjenning in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

runcom · 2020-04-06T14:32:22Z

/cc @yuqi-zhang

LorbusChris · 2020-04-06T14:47:33Z

// +k8s:openapi-gen=true is set in doc.go but and make update yields no further changes in the crd yaml files.
I would've expected it to overwrite the customized validation for machineconfig.spec.config, which is a RawExtension type.
Could somebody clarify why this isn't happening?

abhinavdahiya · 2020-04-06T16:35:53Z

+kind: CustomResourceDefinition
+metadata:
+  # name must match the spec fields below, and be in the form: <plural>.<group>
+  name: controllerconfigs.machineconfiguration.openshift.io


i'm not sure this should be part of the openshift/api since this looks like an machine-config-operator internal API..

they are in the same group at the moment (machineconfiguration.openshift.io). Should we move them out of that group into an MCO-internal group? Are there conventions around this?

abhinavdahiya · 2020-04-06T16:36:45Z

+)
+
+// NewMachineConfigPoolCondition creates a new MachineConfigPool condition.
+func NewMachineConfigPoolCondition(condType MachineConfigPoolConditionType, status corev1.ConditionStatus, reason, message string) *MachineConfigPoolCondition {


hm.. the helper functions usually end up in library-go instead of the API..

looks like they're not used anywhere else and they could stay in the MCO repo for now.

LorbusChris · 2020-04-06T18:56:47Z

Just to verify, the controllerconfigs.machineconfiguration.openshift.io API that is only used by the MachineConfig controller internally should stay in the MCO repo?
If so, should it be moved out of the machineconfiguration API group?

This moves over all external API from the github.com/openshift/machine-config-operator repo, and adds swagger doc generation for it.

LorbusChris · 2020-04-06T19:24:27Z

I've removed the controllerconfig CRD and API from this PR.

Please let me know if I should add it back.
The open questions above remain.

yuqi-zhang · 2020-04-06T22:22:07Z

I'm not very familiar with how we structure CRDs. It'd obviously be nicer from a maintenance perspective if everything in the *.machineconfiguration.openshift.io lived in the same repo but I defer to the experts.

deads2k · 2020-04-07T13:22:58Z

+)
+
+const (
+	GroupName = "machineconfiguration.openshift.io"


this group already exists and this is a straight move?

yes: https://github.com/openshift/machine-config-operator/blob/master/pkg/apis/machineconfiguration.openshift.io/install.go#L9

deads2k · 2020-04-07T13:23:36Z

+    plural: containerruntimeconfigs
+    singular: containerruntimeconfig
+    shortNames:
+    - ctrcfg


is this already present in your CRD manifest today? We discourage shortnames on resources that are infrequently used by cluster-admins.

yes, already present

deads2k · 2020-04-07T13:25:39Z

+
+// MachineConfigRoleLabelKey is metadata key in the MachineConfig. Specifies the node role that config should be applied to.
+// For example: `master` or `worker`
+const MachineConfigRoleLabelKey = "machineconfiguration.openshift.io/role"


general API note. if you own the schema of the struct, why wasn't this just a top level field?

deads2k · 2020-04-07T13:26:20Z

+type MachineConfigPoolSpec struct {
+	// machineConfigSelector specifies a label selector for MachineConfigs.
+	// Refer https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/ on how label and selectors work.
+	MachineConfigSelector *metav1.LabelSelector `json:"machineConfigSelector,omitempty" protobuf:"bytes,1,opt,name=machineConfigSelector"`


you need to explain what nil means here. All, none, somethign else?

deads2k · 2020-04-07T13:26:29Z

+	MachineConfigSelector *metav1.LabelSelector `json:"machineConfigSelector,omitempty" protobuf:"bytes,1,opt,name=machineConfigSelector"`
+
+	// nodeSelector specifies a label selector for Machines
+	NodeSelector *metav1.LabelSelector `json:"nodeSelector,omitempty" protobuf:"bytes,2,opt,name=nodeSelector"`


describe the meaning of nil

deads2k · 2020-04-07T13:26:40Z

+	// +nullable
+	KernelArguments []string `json:"kernelArguments" protobuf:"bytes,3,rep,name=kernelArguments"`
+
+	FIPS       bool   `json:"fips" protobuf:"varint,4,opt,name=fips"`


missing doc

deads2k · 2020-04-07T13:27:01Z

+type MachineConfigSpec struct {
+	// OSImageURL specifies the remote location that will be used to
+	// fetch the OS.
+	OSImageURL string `json:"osImageURL" protobuf:"bytes,1,opt,name=osImageURL"`


missing required/optional if optional, what does empty string mean?

deads2k · 2020-04-07T13:27:20Z

+	// fetch the OS.
+	OSImageURL string `json:"osImageURL" protobuf:"bytes,1,opt,name=osImageURL"`
+	// Config is a Ignition Config object.
+	Config runtime.RawExtension `json:"config" protobuf:"bytes,2,opt,name=config"`


This is fine. You're interpretting the value elsewhere but this eliminated the problematic compile time dep?

yes. This is parsed into Ignition.Config by the MCO

deads2k · 2020-04-07T13:27:32Z

+	// OSImageURL specifies the remote location that will be used to
+	// fetch the OS.
+	OSImageURL string `json:"osImageURL" protobuf:"bytes,1,opt,name=osImageURL"`
+	// Config is a Ignition Config object.


point to where the schema is please.

deads2k · 2020-04-07T13:27:40Z

+	Config runtime.RawExtension `json:"config" protobuf:"bytes,2,opt,name=config"`
+
+	// +nullable
+	KernelArguments []string `json:"kernelArguments" protobuf:"bytes,3,rep,name=kernelArguments"`


deads2k · 2020-04-07T13:28:28Z

+
+	// 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,omitempty" protobuf:"bytes,4,opt,name=maxUnavailable"`


what does zero mean here? based on comment, this would end poorly.

deads2k · 2020-04-07T13:29:37Z

+// MachineConfigPoolStatusConfiguration stores the current configuration for the pool, and
+// optionally also stores the list of MachineConfig objects used to generate the configuration.
+type MachineConfigPoolStatusConfiguration struct {
+	corev1.ObjectReference `json:",inline" protobuf:"bytes,1,opt,name=objectReference"`


You probably don't honor all the fields of this struct. We encourage creating specific reference objects so you can provide precise docs. This will need revisiting.

deads2k · 2020-04-07T13:29:41Z

+
+	// source is the list of MachineConfig objects that were used to generate the single MachineConfig object specified in `content`.
+	// +optional
+	Source []corev1.ObjectReference `json:"source,omitempty" protobuf:"bytes,2,rep,name=source"`


You probably don't honor all the fields of this struct. We encourage creating specific reference objects so you can provide precise docs. This will need revisiting.

deads2k · 2020-04-07T13:30:08Z

+
+// KubeletConfigSpec defines the desired state of KubeletConfig
+type KubeletConfigSpec struct {
+	MachineConfigPoolSelector *metav1.LabelSelector `json:"machineConfigPoolSelector,omitempty" protobuf:"bytes,1,opt,name=machineConfigPoolSelector"`


need to describe the meaning of nil

deads2k · 2020-04-07T13:30:22Z

+// KubeletConfigSpec defines the desired state of KubeletConfig
+type KubeletConfigSpec struct {
+	MachineConfigPoolSelector *metav1.LabelSelector `json:"machineConfigPoolSelector,omitempty" protobuf:"bytes,1,opt,name=machineConfigPoolSelector"`
+	KubeletConfig             *runtime.RawExtension `json:"kubeletConfig,omitempty" protobuf:"bytes,2,opt,name=kubeletConfig"`


describe the meaning of nil and point to what schema is accepted here.

deads2k · 2020-04-07T13:30:53Z

+	KubeletConfigSuccess KubeletConfigStatusConditionType = "Success"
+
+	// KubeletConfigFailure designates a failure applying a KubeletConfig CR.
+	KubeletConfigFailure KubeletConfigStatusConditionType = "Failure"


these conditions are a bit odd, since Failure is usually Success == false.

LorbusChris · 2020-04-07T13:35:38Z

deferring to @runcom to answer these questions. I'd prefer to have these issues fixed in the MCO repo before the move

This removes the hand-rafted API Validation for the RawExtension that contains Ignitition.Config WIP

LorbusChris · 2020-09-23T19:40:21Z

This is stale. Someone from the MCO team should create a new PR once the API move task has been prioritized and scheduled for a sprint.
/close

openshift-ci-robot · 2020-09-23T19:40:37Z

@LorbusChris: Closed this PR.

Details

In response to this:

This is stale. Someone from the MCO team should create a new PR once the API move task has been prioritized and scheduled for a sprint.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested a review from runcom April 6, 2020 14:28

LorbusChris force-pushed the mco branch from 09586ed to 9b164d8 Compare April 6, 2020 14:30

openshift-ci-robot requested a review from yuqi-zhang April 6, 2020 14:32

LorbusChris force-pushed the mco branch from 9b164d8 to 262fc6c Compare April 6, 2020 15:21

abhinavdahiya reviewed Apr 6, 2020

View reviewed changes

Add machineconfiguration API group

c77974a

This moves over all external API from the github.com/openshift/machine-config-operator repo, and adds swagger doc generation for it.

LorbusChris force-pushed the mco branch from 262fc6c to c77974a Compare April 6, 2020 19:05

LorbusChris changed the title ~~Add machineconfiguration API~~ Add machineconfiguration API group Apr 6, 2020

deads2k reviewed Apr 7, 2020

View reviewed changes

Comment thread machineconfiguration/v1/doc.go Outdated

deads2k reviewed Apr 7, 2020

View reviewed changes

WIP fix groupName in docs.go and regenerate

816ad9c

This removes the hand-rafted API Validation for the RawExtension that contains Ignitition.Config WIP

LorbusChris changed the title ~~Add machineconfiguration API group~~ WIP: Add machineconfiguration API group Apr 7, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2020

LorbusChris mentioned this pull request Apr 19, 2020

move types to openshift/api openshift/machine-config-operator#588

Open

openshift-ci-robot closed this Sep 23, 2020

jkyros mentioned this pull request May 4, 2023

MCO-52: add MCO API into openshift/api #1453

Merged

Conversation

LorbusChris commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Apr 6, 2020

Uh oh!

runcom commented Apr 6, 2020

Uh oh!

LorbusChris commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorbusChris commented Apr 6, 2020

Uh oh!

LorbusChris commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuqi-zhang commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorbusChris commented Apr 7, 2020

Uh oh!

LorbusChris commented Sep 23, 2020

Uh oh!

openshift-ci-robot commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LorbusChris commented Apr 6, 2020 •

edited

Loading

LorbusChris commented Apr 6, 2020 •

edited

Loading

LorbusChris commented Apr 6, 2020 •

edited

Loading