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

Don't write to platform spec to allow upgrades #1127

Merged
merged 8 commits into from
Dec 11, 2019

Conversation

nicolaferraro
Copy link
Member

Without this, upgrades to newer versions are not possible, as we used to encode even defaults in the platform specs, but defaults change in every new version (e.g. for instance the default Camel version).

I've moved the full platform config in a fullConfig field in the status, to allow it to be inspected and at the same time to allow the users to set their preference only on what matters for them.

A typical platform on Minikube now looks like:

apiVersion: camel.apache.org/v1alpha1
kind: IntegrationPlatform
metadata:
  creationTimestamp: "2019-12-09T13:56:32Z"
  generation: 1
  labels:
    app: camel-k
  name: camel-k
  namespace: default
  resourceVersion: "562760"
  selfLink: /apis/camel.apache.org/v1alpha1/namespaces/default/integrationplatforms/camel-k
  uid: b4b273c5-1a8b-11ea-8c9d-5cdf4ec533fd
spec:
  build:
    maven:
      settings: {}
    registry:
      address: 10.96.56.193
      insecure: true
  resources: {}
status:
  fullConfig:
    build:
      baseImage: fabric8/s2i-java:3.0-java8
      buildStrategy: pod
      camelVersion: '>=3.0.0-RC3'
      kanikoBuildCache: true
      maven:
        localRepository: /tmp/artifacts/m2
        settings:
          configMapKeyRef:
            key: settings.xml
            name: camel-k-maven-settings
        timeout: 3m45s
      persistentVolumeClaim: camel-k
      publishStrategy: Kaniko
      registry:
        address: 10.96.56.193
        insecure: true
      runtimeVersion: '>=1.0.7'
      timeout: 5m0s
    cluster: Kubernetes
    profile: Knative
  phase: Warming
  version: 1.0.0-M5-SNAPSHOT

Note that only the relevant information (registry address and kind) are present in spec, all other values are computed in the status.

The fullConfig is kept in sync by the platform controller.

Release Note

IntegrationPlatform specification does no longer contain default values in order to ease upgrades

@nicolaferraro nicolaferraro added the kind/feature New feature or request label Dec 9, 2019
@astefanutti
Copy link
Member

@nicolaferraro, just thinking out loud, would using the kubectl.kubernetes.io/last-applied-configuration annotation or something equivalent be a way to transport user defined configuration, e.g., when creating the platform with the apply sub-command:

apiVersion: camel.apache.org/v1alpha1
kind: IntegrationPlatform
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"camel.apache.org/v1alpha1","kind":"IntegrationPlatform","metadata":{"annotations":{},"labels":{"app":"camel-k"},"name":"camel-k","namespace":"camel-k"}}

@nicolaferraro
Copy link
Member Author

Yeah, I've thought to the last-applied-config as well, but semantically in Kube it serves to a different purpose. Having it as an annotation also make it difficult to understand the content when you inspect it.

It's also difficult, in case one changes with k edit integrationplatform some fields in the spec, when you get to reconcile it, to tell if a value in spec was set by the user or by our initialization code.

I think we've done a similar thing to this PR also in Integration (e.g. you see many fields, like dependencies in both status and spec). Instead of having a fullConfig field, which seems odd, I was thinking to move the fields one level up. So that we have e.g. spec->build->baseImage and status->build->baseImage which seems close to what we've already done in integration and feels more natural.

@astefanutti
Copy link
Member

I think we've done a similar thing to this PR also in Integration (e.g. you see many fields, like dependencies in both status and spec). Instead of having a fullConfig field, which seems odd, I was thinking to move the fields one level up. So that we have e.g. spec->build->baseImage and status->build->baseImage which seems close to what we've already done in integration and feels more natural.

Totally agree with this.

@nicolaferraro
Copy link
Member Author

Looks much nicer now:

apiVersion: camel.apache.org/v1alpha1
kind: IntegrationPlatform
metadata:
  creationTimestamp: "2019-12-10T10:24:47Z"
  generation: 1
  labels:
    app: camel-k
  name: camel-k
  namespace: default
  resourceVersion: "655195"
  selfLink: /apis/camel.apache.org/v1alpha1/namespaces/default/integrationplatforms/camel-k
  uid: 4a4e7727-1b37-11ea-8c9d-5cdf4ec533fd
spec:
  build:
    maven:
      settings: {}
    registry:
      address: 10.96.56.193
      insecure: true
  resources: {}
status:
  build:
    baseImage: fabric8/s2i-java:3.0-java8
    buildStrategy: pod
    camelVersion: '>=3.0.0-RC3'
    kanikoBuildCache: true
    maven:
      localRepository: /tmp/artifacts/m2
      settings:
        configMapKeyRef:
          key: settings.xml
          name: camel-k-maven-settings
      timeout: 3m45s
    persistentVolumeClaim: camel-k
    publishStrategy: Kaniko
    registry:
      address: 10.96.56.193
      insecure: true
    runtimeVersion: '>=1.0.7'
    timeout: 5m0s
  cluster: Kubernetes
  phase: Warming
  profile: Knative
  version: 1.0.0-M5-SNAPSHOT

@@ -25,6 +25,7 @@ import (

"github.com/apache/camel-k/pkg/util/registry"
"go.uber.org/multierr"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Copy link
Member

Choose a reason for hiding this comment

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

nit: use the metav1 alias as it's used elsewhere.

pkg/cmd/install.go Show resolved Hide resolved
@astefanutti
Copy link
Member

LGTM!

@nicolaferraro nicolaferraro merged commit 5b0754b into apache:master Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants