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

Support decoupling operator and installed software versions #56

Closed
evankanderson opened this issue Apr 30, 2020 · 17 comments
Closed

Support decoupling operator and installed software versions #56

evankanderson opened this issue Apr 30, 2020 · 17 comments
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@evankanderson
Copy link
Member

(copied from knative/serving-operator#301)

Problem
Today, the serving yaml is packaged and read from the .kodata file in the serving-operator image. This has several negative consequences:

  1. It's hard to package an operator that installs nightly
  2. If serving needs a cherry-pick or patch, the serving operator needs to cut a new release, too.
  3. We don't have a good record of what we previously installed in the cluster.

Possibly-related issues: knative/serving-operator#277 knative/serving-operator#276 knative/serving-operator#256

Persona:
Which persona is this feature for?
This is for contributors (who will be able to use the serving operator during development) and for operators.

Exit Criteria

  1. The operator controller can deploy a newer version of Knative than it was built for.
  2. The operator controller can downgrade fom a newer version of Knative to an older one
  3. The operator controller can upgrade or downgrade across multiple versioned releases at one time and the operator coordinates the change.

Time Estimate (optional):
Probably 1-2 months for 1-2 engineers.

Additional context (optional)
Proposal:

https://docs.google.com/document/d/1ehd5Yx4aSottuv7FYW4JeplNULJCs7nnDfy29y3NH8k/edit#

@evankanderson
Copy link
Member Author

Questions from @houshengbo:

I have the following questions regarding this feature:

How does the operator validate the customized manifest, as a certain or a few supported versions?

As the manifest of serving have been split into serving-crds, serving-core, serving-hpa, net-istio, do we allow all of them customizable at the same time? If not, which ones do we allow for partial customization? How to do partial customization?

@evankanderson
Copy link
Member Author

I'll work these into the doc by Monday

@houshengbo
Copy link
Contributor

Once users directly gain the power of customize yaml with any options, how valuable is the operator API still?
It will surely become the ultimate solution to any user issue.

@houshengbo
Copy link
Contributor

houshengbo commented May 8, 2020

I add the following content as a comment to the proposal as well.

How about using this feature as an easy first step with following configuration options?

  1. Install the official Knative with a specific version.
    This configuration means NO customization on any official knative yaml files. Operator can automatically pull the official knative releases, coz we know the links. Users just specify a version, e.g. 0.14.0.
    We can define the CR like this:
spec:
    manifests:
        version: 0.14.0
  1. Install users' own Knative yamls WITH a version.
    Maybe I am wrong, but I still see it valuable to specify the version, since our CRs indicate the version. We need to have somewhere CR is able to read the version. Users MUST be responsible for the match between the YAMLS and the version they specify.
    We define the CR like this:
spec:
    manifests:
        version: 0.14.0
        - sourceUrl: https://<my-own-url>/serving-ALL.yaml

Users can consolidate the contents into one yaml, like serving-ALL.yaml.
If they choose to save them into multiple files, like serving-core.yaml,
serving-sth-else.yaml... They can define CR like this:

spec:
    manifests:
        version: 0.14.0
        - sourceUrl: https://<my-own-url>/serving-core.yaml, https://<my-own-url>/serving-sth-else.yaml

The package manifestival is able to parse comma-separated yaml links

OR

spec:
    manifests:
        version: 0.14.0
        - sourceUrl: https://<my-own-url>/serving-core.yaml
        - sourceUrl: https://<my-own-url>/serving-sth-else.yaml

When we firstly get this done, they can add version validation later. To me, this little first step is good enough to call in release 0.15. :-)

@evankanderson @markusthoemmes
What do you think?

@markusthoemmes
Copy link
Contributor

I agree with the sentiment, yes. The API surface needs some bikeshedding though.

@evankanderson
Copy link
Member Author

Yeah -- I'm not sure that declaring "version" in the custom object when pointing to a manifest is the correct approach, since the manifest might actually be for some other version.

Also, if manifests is a list (as the name implies), then I'd expect a list of objects, each with a single sourceUrl. Right now, your manifests seems to be both a list and an object, which I'm not sure is valid JSON or YAML.

@houshengbo
Copy link
Contributor

For the sourceUrl, we can stay with the list. The object of comma-separated yanl files is just an option for the manifestival package, since it is a supported format to input yamls.

If we only specify the links of the yamls, how can the operator CR get the version or build number? So far the version is hard-coded here: https://github.com/knative-sandbox/operator/blob/master/version/version.go#L3 This was the reason that user may need to specify the version.

@jcrossley3
Copy link
Contributor

Do we really need the "sourceUrl:" prefix for every item in the list? Seems redundant.

@houshengbo
Copy link
Contributor

@evankanderson @jcrossley3 Do we have a way to retrieve the version number, by parsing the manifest of serving?
e.g. Reading all over the resources, and locating the label serving.knative.dev/release?
Can we get the version number this way?

@houshengbo
Copy link
Contributor

@jcrossley3 sourceUrl is the field name. My understanding is that we may need different field names to specify different urls.
sourceUrl is used for url of knative components.
xxxUrl is used for url of network ingress...

@evankanderson
Copy link
Member Author

My thought was that having a list of objects rather than a list of URLs would allow additional fields (such as an expected checksum of the object).

I think we have version information as annotations on specific resources. What's the use case for knowing the version for the entire yaml file? (I ask so that we can focus on putting the information in the right place -- maybe in the CR or maybe in the YAML files.)

@evankanderson
Copy link
Member Author

(oops, tab-enter did the wrong thing)

@jcrossley3
Copy link
Contributor

Additional fields, e.g. checksum, makes sense, thanks. I'd still prefer url or path just to conserve space, though ;)

@houshengbo
Copy link
Contributor

houshengbo commented May 12, 2020

@evankanderson The operator's CRs need to fill in the version number. If we specify the yamls by url, where to get this version?
https://github.com/knative-sandbox/operator/blob/eec6b1b2a372159c4f992686c71d363849171b23/pkg/reconciler/common/install.go#L50

@markusthoemmes
Copy link
Contributor

markusthoemmes commented May 12, 2020

In my mind, the API looks roughly like this:

type KnativeServingSpec struct {
  // Specifies which version of KnativeServing will be installed.
  version string

  // Overrides the loaded manifest.
  // +optional
  manifest []Manifest
}

type Manifest struct {
  // A url to point to the manifest to load. Can also start with file:// to load local files.
  url string

  // Just an example...
  checksum string
}

Which then allow for the YAML to look like this:

Default

spec:
  version: 0.14.2

Overridden

spec:
  version: 0.14.2
  manifest:
    - url: https://foo.bar/0.14.2/core.yaml
    - url: https://foo.bar/0.14.2/extras.yaml

Overridden with checksums

spec:
  version: 0.14.2
  manifest:
    - url: https://foo.bar/0.14.2/core.yaml
      checksum: abcd
    - url: https://foo.bar/0.14.2/extras.yaml
      checksum: abcd

@markusthoemmes
Copy link
Contributor

@houshengbo the version always needs to be specified in the spec I think. It should not be inferred from the YAML I think.

@github-actions
Copy link
Contributor

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 17, 2020
matzew added a commit to matzew/knative-operator that referenced this issue Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants