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.

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

Add openshift/4.8.0-exp that produces MachineConfigs by default #212

Merged
merged 6 commits into from
Mar 19, 2021
Merged

Add openshift/4.8.0-exp that produces MachineConfigs by default #212

merged 6 commits into from
Mar 19, 2021

Conversation

bgilbert
Copy link
Contributor

Rename rhcos/0.2.0-experimental to openshift/4.8.0-experimental. Use openshift as the variant name instead of rhcos because RHCOS is an implementation detail of OpenShift, and also would leave OKD in an awkward position. By reusing OpenShift version numbers, we're setting ourselves up to eventually violate semver, but it doesn't seem reasonable to create a novel set of user-facing version numbers that isn't connected to anything else.

Have the new spec generate MachineConfigs by default, but also accept a new -r/--raw/Raw option to generate an Ignition config. This allows FCCT to be used for sugar (e.g. for boot disk mirroring) and validation (e.g. rejecting users other than core) without modifying the pointer config, and without requiring MCO changes. We'll have a new spec version for each OpenShift release, so we'll also have the ability to change the output format as needed by future MCO releases.

As an implementation detail, ship a simplified copy of the MachineConfig structs so we don't have to pull in a bunch of k8s code as dependencies. (And to avoid a circular dependency with the MCO.)

Sample input.occ:

variant: openshift
version: 4.8.0-experimental
metadata:
  name: create-etc-sample
  labels:
    machineconfiguration.openshift.io/role: worker
storage:
  files:
    - path: /etc/sample
      contents:
        inline: hello world

fcct input.occ produces:

apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
  labels:
    machineconfiguration.openshift.io/role: worker
  name: create-etc-sample
spec:
  config:
    ignition:
      version: 3.2.0
    storage:
      files:
      - contents:
          source: data:,hello%20world
        path: /etc/sample

And fcct --pretty --raw input.occ produces:

{
  "ignition": {
    "version": "3.2.0"
  },
  "storage": {
    "files": [
      {
        "path": "/etc/sample",
        "contents": {
          "source": "data:,hello%20world"
        }
      }
    ]
  }
}

@yuqi-zhang, @sinnykumari, and/or other folks from the MCO team, wdyt?

We want to support both OCP and OKD.
We're not going to switch to spec 3.3.0 in this cycle.
Don't use it for anything yet, but require that metadata.name is
specified.
@sinnykumari
Copy link

I don't have full context here, is the usecase of this is that user can write openshift coreos config (yaml config) and can use fcct tool to generate MachineConfig?

cc @kikisdeliveryservice

@bgilbert
Copy link
Contributor Author

@sinnykumari Yup, that's right. For example, it would let us convert the mirrored disk instructions to use a MachineConfig manifest. Other workflows in the customization docs could also be switched to preprocess through fcct in cases where its sugar and validation capabilities would be useful.

@yuqi-zhang
Copy link

In the MCO today I believe we use FCCT to render our templates into machineconfigs, although we make no explicit reference to the schema type. Is that something we should change? Or is that perhaps unrelated to this change

@sinnykumari
Copy link

@sinnykumari Yup, that's right. For example, it would let us convert the mirrored disk instructions to use a MachineConfig manifest.

+1

Other workflows in the customization docs could also be switched to preprocess through fcct in cases where its sugar and validation capabilities would be useful.

agree, for better user experience it would be nice to be able to generate rest of MachineConfig file fields as well.
It makes me think renaming the tool from fcct to more generic would be useful as this will get used by FCOS, OKD and OCP.

And current .occ format looks fine to me.

@LorbusChris
Copy link
Contributor

+1 from my side as well.

Just to xref some previous ideas in this area:

Do I see this correctly and this RFC will be able tp obsolete these approaches?

@bgilbert
Copy link
Contributor Author

In the MCO today I believe we use FCCT to render our templates into machineconfigs, although we make no explicit reference to the schema type. Is that something we should change? Or is that perhaps unrelated to this change

MCO is using FCCT in a limited way to transpile file and unit sections, but not to transpile entire configs. It should be fine to continue as is for now; there's no harm to passing desugared config through FCCT twice.

for better user experience it would be nice to be able to generate rest of MachineConfig file fields as well.

Yup, we can do that as a followup.

It makes me think renaming the tool from fcct to more generic would be useful as this will get used by FCOS, OKD and OCP.

We're planning to do that before 4.8, currently blocked on finding a good name. If anyone has any ideas, please add them to #167.

Do I see this correctly and this RFC will be able tp obsolete these approaches?

I don't think they're incompatible. This RFC lets us incrementally improve our config handling without MCO changes. In the longer term, we could still reconsider the CRD format, which would allow us to avoid asking users to preprocess their configs with an external tool.

@bgilbert bgilbert changed the title RFC: Add openshift/4.8.0-exp that produces MachineConfigs by default Add openshift/4.8.0-exp that produces MachineConfigs by default Mar 13, 2021
@bgilbert
Copy link
Contributor Author

No major objections raised so far; dropping RFC!

@ashcrow ashcrow mentioned this pull request Mar 15, 2021
@bgilbert
Copy link
Contributor Author

This part of the new FCC syntax is worth some thought:

metadata:
  labels:
    machineconfiguration.openshift.io/role: worker

Nowhere else in the FCC or Ignition specs do we allow user-specified map keys. Normally we'd implement this functionality like so:

metadata:
  labels:
    - key: machineconfiguration.openshift.io/role
      value: worker

I'm concerned that if we use a datatype here that we don't use elsewhere, and don't plan to use elsewhere, that it will cause complications. It already introduces a presentation issue in documentation and requires some additional code in translate/util.go. On the other hand, a more Ignition-like structure would be inconsistent with k8s norms.

Thoughts?

@arithx
Copy link
Contributor

arithx commented Mar 17, 2021

I would lean towards the former example. I think making it clear that we're directly passing those arguments through into the resulting MachineConfig is sufficient (as well giving us the mentioned benefit of matching the k8s norms).

Copy link
Contributor

@arithx arithx left a comment

Choose a reason for hiding this comment

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

LGTM

internal/main.go Outdated
@@ -45,6 +45,7 @@ func main() {
pflag.Lookup("debug").Hidden = true
pflag.BoolVarP(&options.Strict, "strict", "s", false, "fail on any warning")
pflag.BoolVarP(&options.Pretty, "pretty", "p", false, "output formatted json")
pflag.BoolVarP(&options.Raw, "raw", "r", false, "output only Ignition config, without any wrapper")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should mention this is only valid on openshift variants or maybe throw a warning/error if we see it set in other variants? I'm not sure that without any wrapper means anything without context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to never wrap in a MachineConfig; force Ignition output. For the other variants, we always output raw Ignition, so it feels odd to complain if the user redundantly requests it.

@jlebon
Copy link
Member

jlebon commented Mar 17, 2021

This part of the new FCC syntax is worth some thought:

Agreed with @arithx. Just matching the k8s semantics would be a nicer UX. Documentation-wise, feels like we can just leave out the awkward arbitrary-key line entirely and enhance the labels line to something like:

  • labels (object): string key/value pairs to apply as [Kubernetes labels][k8s-labels] to this MachineConfig.

?

@arithx
Copy link
Contributor

arithx commented Mar 17, 2021

Documentation-wise, feels like we can just leave out the awkward arbitrary-key line entirely and enhance the labels line to something like:

  • labels (map[string]string): string key/value pairs to apply as [Kubernetes labels][k8s-labels] to this MachineConfig.

Could also do something like this, describing the type as a map[string]string

Generate a MachineConfig by default, but produce an Ignition config if
the Raw/-r/--raw translation option is specified.

We host our own reimplementation of MachineConfig structs to avoid
pulling in a massive (and circular) set of dependencies from
openshift/machine-config-operator.
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.

7 participants