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 conditions to camel-k CRs #766

Merged
merged 2 commits into from
Jul 10, 2019
Merged

Conversation

lburgazzoli
Copy link
Contributor

@lburgazzoli lburgazzoli commented Jun 23, 2019

Fixes #594

I did a little bit of work to introduce conditions in camel-k CRs so now if you invoke

kamel describe integration hello

you'd get an output like:

Name:                hello
Namespace:           myproject
Creation Timestamp:  Sun, 23 Jun 2019 11:46:00 +0200
Phase:               Running
Camel Version:       2.24.0
Kit:                 kit-bk7kh138gd7vch0m1d00
Image:               172.30.1.1:5000/myproject/camel-k-kit-bk7kh138gd7vch0m1d00:843534
Dependencies:
  camel:core
  runtime:jvm
Sources:
  Name       Language  Compression  Ref  Ref Key
  hello.xml  xml       false             
Conditions:
  Type                 Status  Reason                        Message
  PlatformAvailable    True    IntegrationPlatformAvailable  IntegrationPlatform (camel-k)
  KitAvailable         True    IntegrationKitAvailable       IntegrationKit (kit-bk7kh138gd7vch0m1d00)
  DeploymentAvailable  True    Deployment                    Deployment
  ExposureAvailable    True    Service                       Service
Traits:
  Route:
    Configuration:
      Enabled:  false
  Service:
    Configuration:
      Auto:     false
      Enabled:  true

which includes a number of conditions that represents the latest available observations of a integration state.

The goal here is to provide more insight about the integration status i.e. the additional resources associated to the integration that the operator has created (deployment vs knative service, service/route/ingress, etc) so user and tools do not need to explore the integration namespace to know what are the other resources related to the integration.

@astefanutti @nicolaferraro does this make sense ?

Copy link
Member

@nicolaferraro nicolaferraro left a comment

Choose a reason for hiding this comment

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

Like it!

@lburgazzoli lburgazzoli force-pushed the github-594 branch 5 times, most recently from cc05ef9 to 66c8e5c Compare July 4, 2019 12:53
Copy link
Member

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

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

Love it!

Two general comments:

  • Currently, conditions are shared among CRs, so it is possible to mix them, I wonder whether there should be dedicated conditions types per CR
  • It seems there is a larger need to report the owned resources and I wonder to what extend it generalises the proposed DeploymentAvailable and ExposureAvailable conditions

@lburgazzoli
Copy link
Contributor Author

Love it!

Two general comments:

  • Currently, conditions are shared among CRs, so it is possible to mix them, I wonder whether there should be dedicated conditions types per CR

Yes, that’s what I’m working on. It is a little bit of a pain as we basically need to duplicate all the code but that’t the way to go :)

  • It seems there is a larger need to report the owned resources and I wonder to what extend it generalises the proposed DeploymentAvailable and ExposureAvailable conditions

Yes, the example here is just a placeholder to show how it will look but the final goal is to report any owned resource

@lburgazzoli lburgazzoli force-pushed the github-594 branch 3 times, most recently from 08742f4 to e67fc49 Compare July 7, 2019 15:23
@lburgazzoli
Copy link
Contributor Author

lburgazzoli commented Jul 8, 2019

So I've added a little bit more logic and now if you describe an integration while it is building a kit you'll see something like:

Name:                routes-rest
Namespace:           myproject
Creation Timestamp:  Mon, 08 Jul 2019 13:34:02 +0200
Phase:               Building Kit
Camel Version:       3.0.0-M3
Kit:                 kit-bkhikar8gd7uoo13f47g
Image:               
Version:             1.0.0-M1-SNAPSHOT
Dependencies:
  camel:core
  camel:log
  camel:timer
  camel:undertow
  runtime:jvm
Sources:
  Name            Language  Compression  Ref  Ref Key
  routes-rest.js  js        false             
Conditions:
  Type                          Status  Reason                        Message
  IntegrationPlatformAvailable  True    IntegrationPlatformAvailable  camel-k
  IntegrationKitAvailable       False   IntegrationKitAvailable       

Once the kit is ready, then the IntegrationKitAvailable condition become true and other info such as services an routes are reported:

Name:                routes-rest
Namespace:           myproject
Creation Timestamp:  Mon, 08 Jul 2019 13:34:02 +0200
Phase:               Running
Camel Version:       3.0.0-M3
Kit:                 kit-bkhikar8gd7uoo13f47g
Image:               172.30.1.1:5000/myproject/camel-k-kit-bkhikar8gd7uoo13f47g:1281056
Version:             1.0.0-M1-SNAPSHOT
Dependencies:
  camel:core
  camel:log
  camel:timer
  camel:undertow
  runtime:jvm
Sources:
  Name            Language  Compression  Ref  Ref Key
  routes-rest.js  js        false             
Conditions:
  Type                          Status  Reason                        Message
  IntegrationPlatformAvailable  True    IntegrationPlatformAvailable  camel-k
  IntegrationKitAvailable       True    IntegrationKitAvailable       kit-bkhikar8gd7uoo13f47g
  DeploymentAvailable           True    DeploymentAvailable           routes-rest
  ExposureAvailable             True    RouteAvailable                routes-rest -> routes-rest(http)
  ServiceAvailable              True    ServiceAvailable              routes-rest(http/80) -> routes-rest(http/8080)

If you run the same integration but with the service trait disabled, you should see something like:

Name:                routes-rest
Namespace:           myproject
Creation Timestamp:  Mon, 08 Jul 2019 13:37:12 +0200
Phase:               Running
Camel Version:       3.0.0-M3
Kit:                 kit-bkhikar8gd7uoo13f47g
Image:               172.30.1.1:5000/myproject/camel-k-kit-bkhikar8gd7uoo13f47g:1281056
Version:             1.0.0-M1-SNAPSHOT
Dependencies:
  camel:core
  camel:log
  camel:timer
  camel:undertow
  runtime:jvm
Sources:
  Name            Language  Compression  Ref  Ref Key
  routes-rest.js  js        false             
Conditions:
  Type                          Status  Reason                        Message
  IntegrationPlatformAvailable  True    IntegrationPlatformAvailable  camel-k
  ServiceAvailable              False   ServiceNotAvailable           explicitly disabled
  IntegrationKitAvailable       True    IntegrationKitAvailable       kit-bkhikar8gd7uoo13f47g
  DeploymentAvailable           True    DeploymentAvailable           routes-rest
  ExposureAvailable             False   RouteNotAvailable             no target service found

@lburgazzoli lburgazzoli marked this pull request as ready for review July 8, 2019 11:40
@lburgazzoli
Copy link
Contributor Author

lburgazzoli commented Jul 8, 2019

@astefanutti @nicolaferraro we still miss to additional information such as the config maps that the integration creates/requires among the conditions but didn't want to make the PR too big, can you please review ?

@@ -78,6 +79,14 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
UpdateFunc: func(e event.UpdateEvent) bool {
oldIntegration := e.ObjectOld.(*v1alpha1.Integration)
newIntegration := e.ObjectNew.(*v1alpha1.Integration)

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that this statement is already logged in the Reconcile method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah, that's a leftover from the rebase, will fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@astefanutti
Copy link
Member

I wonder if it'd be possible to have a base type for the conditions that would enable factorising some of the helper methods as well.

@lburgazzoli
Copy link
Contributor Author

yeah, who needs generics ? :P
I can have a look but haven't found any concrete example so fa on kubernetes for a best practice

@astefanutti
Copy link
Member

Let's wait for go to have generics 😄

@lburgazzoli
Copy link
Contributor Author

lburgazzoli commented Jul 10, 2019

@astefanutti can this be merged ?

@astefanutti
Copy link
Member

Good for me 😉

@lburgazzoli lburgazzoli merged commit cc339f7 into apache:master Jul 10, 2019
@lburgazzoli lburgazzoli deleted the github-594 branch July 10, 2019 14:40
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.

Add conditions to camel-k CRs
3 participants