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

fix(5330): Moves the todo app to addons directory #5399

Closed
wants to merge 1 commit into from

Conversation

phantomjinx
Copy link
Contributor

  • syndesis.yml

  • Strips out sampledb & todo & relocates them to install/addons

  • .../commands/[install|util/openshift_funcs]

  • Adds a --addons switch that takes a comma-separated list of addons to
    install in addition to base syndesis components. At the moment this
    list would be "db-sampledb,todo"

  • main.go

  • Adds in default of /conf/addons to turn on addons support

  • syndesis_types.go

  • Adds additional fields to structs for extra environment variables, inc.
    Addons (which contains the preferred set of addons)

  • templateprocessor.go

  • To fully support sampledb and todo moving to addons, the env vars from
    the syndesis template must be available. This is due to the sample-db
    being a full copy of the syndesis-db resource with the extra sample-db
    elements included.

  • action/[install|upgrade].go

  • When installing addons:

  • Check their is some in the directory
  • Find the addon resources and get each addon ID
  • If addon ID is included in the list specified by --addons switch then
    install
  • addon/install.go

  • Both sampledb & todo have env vars that must have values injected in
    them before the resource is loaded. The template has the templateprocessor
    that does the same. loadAndProcessFile() conducts a string replace over
    each file ensuring env vars are correctly substituted for their values

  • Since the metadata.name of each resource is not unique then it is
    essential that each resource has a managed ID that can be used to
    select it for install. The syndesis.io/addon label provides this. It
    doesn't need to be unique as all the todo resources have the same ID.

  • configuration.go

  • Add in extra env vars required by addons, inc. addons

* syndesis.yml
 * Strips out sampledb & todo & relocates them to install/addons

* .../commands/[install|util/openshift_funcs]
 * Adds a --addons switch that takes a comma-separated list of addons to
   install in addition to base syndesis components. At the moment this
   list would be "db-sampledb,todo"

* main.go
 * Adds in default of /conf/addons to turn on addons support

* syndesis_types.go
 * Adds additional fields to structs for extra environment variables, inc.
   Addons (which contains the preferred set of addons)

* templateprocessor.go
 * To fully support sampledb and todo moving to addons, the env vars from
   the syndesis template must be available. This is due to the sample-db
   being a full copy of the syndesis-db resource with the extra sample-db
   elements included.

* action/[install|upgrade].go
 * When installing addons:
  - Check their is some in the directory
  - Find the addon resources and get each addon ID
  - If addon ID is included in the list specified by --addons switch then
    install

* addon/install.go
 * Both sampledb & todo have env vars that must have values injected in
   them before the resource is loaded. The template has the templateprocessor
   that does the same. loadAndProcessFile() conducts a string replace over
   each file ensuring env vars are correctly substituted for their values
 * Since the metadata.name of each resource is not unique then it is
   essential that each resource has a managed ID that can be used to
   select it for install. The syndesis.io/addon label provides this. It
   doesn't need to be unique as all the todo resources have the same ID.

* configuration.go
 * Add in extra env vars required by addons, inc. addons
@phantomjinx phantomjinx added the pr/wip Mark work-in-progress with this label. pure-bot won't merge a PR if this is set. label May 17, 2019
@pure-bot pure-bot bot added the pr/review-requested Use this if you want to have a review. pure-bot will prevent merging if set and no review given label May 17, 2019
@phantomjinx
Copy link
Contributor Author

I wanted to get this out there for people to look at & see if its the right way forward. Essentially, the 'todo' & 'sample-db' resources have been moved from syndesis.yml to addons/ . This implies:

  • syndesis install will not install any addons at all so no todo or sample-db;
  • syndesis install --addons db-sampledb,todo will install everything;

To test:

  1. Update src repository;
  2. Create a clean project;
  3. Ensure that syndesis is in $GOLANG path (I softlink syndesis src directory -> ~/go/src/github.com/syndesisio/syndesis);
  4. Execute syndesis build --local -m operator -f -i which should result in a completed syndesis-operator-build pod;
  5. Execute either
  • syndesis install --local --addons db-sampledb,todo which should result in everything being installed;
  • syndesis install --local which should result in syndesis being installed sans sample-db & todo app.

This updates the syndesis install tool but not yet touches:

  • syndesis install shell script;
  • fuse-online shell script;
  • mustache generator that creates syndesis.yml
  • consider what else needs updating??

@heiko-braun @gaughan

@rhuss rhuss removed their request for review May 17, 2019 16:41
@zregvart
Copy link
Member

Cool stuff, couple of thoughts:

  • I'd prefer each add-on to be in a single file (merge syndesis-sampledb-*.yml, syndesis-todo-*.yml, and as the todo app depends on the sampledb might be better to put them together in a single pod
  • I'd move the passwords in DbConfiguration to a Secret
  • perhaps processing the template using OpenShift template might be a better solution than to process it manually in loadAndProcessFile, not sure what we would like to do in the long run when we remove all templates, probably transfer those templates into golang structures and simply reference the field from the Syndesis custom resource

@phantomjinx
Copy link
Contributor Author

* I'd prefer each add-on to be in a single file (merge `syndesis-sampledb-*.yml`, `syndesis-todo-*.yml`, and as the todo app depends on the sampledb might be better to put them together in a single pod

* I'd move the passwords in `DbConfiguration` to a `Secret`

* perhaps processing the template using OpenShift template might be a better solution than to process it manually in `loadAndProcessFile`, not sure what we would like to do in the long run when we remove all templates, probably transfer those templates into golang structures and simply reference the field from the Syndesis custom resource

So some of this was a little unfamiliar to me when I started. Looking at the code, the syndesis template was being loaded using OS-template whereas the addons were not. Then I found that the addons were in fact Custom Resource Definitions which are yaml but not templates. The existing addons appeared to be 1 resource per file which seemed to necessitate todo and sample-db being separate files. Likewise, being now CRDs rather than templates again meant no recourse to using OS-template & plugging in config vars manually. All that being said:

  • Can CRDs be understood by OS-template functions?
  • Should todo and sample-db be treated as CRDs while other addons are CRDs?
  • should todo & sample-db be migrated to golang structure as is suggested is the long-term goal and proper CRDs be created instead?

@zregvart

@zregvart
Copy link
Member

  • Can CRDs be understood by OS-template functions?

If I understood the question correctly: CRDs behave much the same as any other (built-in or not) k8s object and they can be part of templates. Though I'm not sure if we wish to be tied to templates in the long run, making those bits a template can be the first step, or we can decide no more templates (or YAML files for that matter) and only operator is the only way to go.

  • Should todo and sample-db be treated as CRDs while other addons are CRDs?

I'm not sure what you mean, the only CRD we have is the Syndesis CRD, other YAML files are either OpenShift template with k8s/OpenShift objects. If I squint and try to see (I could be wrong) what you're asking I'd group todo and sample-db in a single YAML file and in a single Pod.

  • should todo & sample-db be migrated to golang structure as is suggested is the long-term goal and proper CRDs be created instead?

Well, yeah, that would be the long/mid term goal, we cannot proceed with that as long as we have a dependency of other systems on our templates. And one of these is the trial environment where we install via templates only. I think for us it would be much simpler to do everything in the operator, we would have much finer control over what add-ons are created and could react on when they're created to perform additional logic. For example, when sample DB is created we can create the database connection in Syndesis.

@phantomjinx
Copy link
Contributor Author

@zregvart
I'll try to clarify a bit as my comment was vague ...

When the operator executes it kicks off by loading the syndesis template by calling GetInstallResourcesAsRuntimeObjects(). This function executes GetInstallResources(), which loads the template -> casts it to a Template* -> runs it through the TemplateProcessor (which sends it to Openshift server and back).

The addons are handled in a similar but not identical fashion. There were already a collection of CRDs concerning monitoring etc... (available here). These were already loaded copied into the syndesis-operator image at built time. I simply added in '/conf/addons' as a default directory to actually attempt to load them. Interestingly, I wasn't able to get any of them to work.

So GetAddonsResources() leads to util.LoadUnstructuredObjectFromFile() which returns each addon as an unstructed* object and no processing is performed. None of the existing addons contain any EnvVars hence such processing has never been an issue (also not having been plugged in yet either).

Therefore,

  • Are the current addons in the addons directory to be retained?
  • The current addons are CRDs whereas my todo & sample-db do not appear to be??
  • Should all addons be CRDs or should they be a mix of CRDs & templates; should they / can they be processed in the same fashion?
  • So going forward we would like to make todo & sample-db 'proper' CRDs but we cannot move totally just yet due to some legacy systems (but we can have the operator implement this but need to keep the templates around as well). Is this correct?

Sorry, if that doesn't move the conversation along. Please ping me direct if you think it necessary.

@zregvart
Copy link
Member

* Are the current addons in the addons directory to be retained?

Yes, I think they are there to add functionality which we should not loose :)

@jamesnetherton @alexkieling @lgarciaaco would know better of any plans to get those addons in the the operator, I think that would be marvellous.

* The current addons are CRDs whereas my todo & sample-db do not appear to be??

Don't get caught up in this, we can consider them YAML files that hold definitions of Kubernetes objects, whatever is there we should consider to be an addon, regardless of if it's a collection of objects, OpenShift template or a custom resource.

The ideal scenario would be that we can drop any Kubernetes, OpenShift or supported custom resource in YAML format there and Syndesis operator could create those if the user chooses/configures the Syndesis custom resource to do that.

* Should all addons be CRDs or should they be a mix of CRDs & templates; should they / can they be processed in the same fashion?

Here's what I would do, detect the type of resource in the addons directory, for templates process them and create objects from the template (similar to how we do it for the syndesis.yml), for other objects create them as is.

For this to work we need to import additional schemes to our Kubernetes API client. And this can be done something like this for Integreatly Graphana custom resource example:

import (
	"github.com/operator-framework/operator-sdk/pkg/util/k8sutil"
	graphana "github.com/integr8ly/grafana-operator/pkg/apis"
)

//...
k8sutil.AddToSDKScheme(graphana.AddToScheme)
* So going forward we would like to make todo & sample-db 'proper' CRDs but we cannot move totally just yet due to some legacy systems (but we can have the operator implement this but need to keep the templates around as well). Is this correct?

I think creating a custom resource for every add-on is probably an overkil, supporting the existing custom resources and adding support for OpenShift templates in addons I think is sufficient.

Perhaps start with the simplest thing, supporting just the existing Kubernetes and OpenShift objects, the only thing we need for todo app, and then extending the support for Integreatly custom resources and templates.

Thank you for doing this work, we are bit behind the curve (and we were ahead) and have neglected the operator a bit 👍.

Oh, and you'll probably find that the operatorsdk needs updating, so that's another task (I think we have an issue opened for it).

@abkieling
Copy link
Contributor

@phantomjinx @zregvart The addons directory was originally created to store Grafana dashboards and alerts used by the Integr8ly monitoring solution. The plan was to create those CRDs only when the monitoring solution is available. Other files can also be added there. The solution isn't specific to monitoring.

@heiko-braun
Copy link
Collaborator

@lburgazzoli @nicolaferraro What do you think about this idea?

@heiko-braun
Copy link
Collaborator

@phantomjinx I like this idea, but think we should not introduce it this late and better wait for 7.5. Can we keep it out of master until we have created the branch for 7.4 (syndesis 1.7.x, ~3 weeks)? I would like to prevent any side effects that might cause issues in productisation.

@nicolaferraro
Copy link
Contributor

The idea of managing addons using config in the syndesis custom resource is really cool.
The concern that I have is about managing change. We can do great things during first installation, but for example it's difficult to use the same approach to react on change (i.e. installing the sampledb when the user changes the syndesis custom resource and enables the addon, uninstalling it when the addon is disabled by the user), which is next level in the operator maturity model.

Another concern is about upgrades. Our current approach is mostly manual, and we spin up a new upgrade pod that basically uses the syndesis.yml as reference to understand what should be upgraded. If we remove e.g. the todo-app from syndesis.yml, I don't know what happens when we do an upgrade. Best thing I can imagine is that the todo-app won't be upgraded (if we change something in the definition). Worst thing is that it won't be recreated. Unless we fix also the upgrade process to deal with addons...

I agree we should try to create a comprehensive solution, probably in 7.5, that handles installation, upgrades and configuration change using the same model. But having too many ways to install syndesis does not help.

@phantomjinx
Copy link
Contributor Author

Another concern is about upgrades. Our current approach is mostly manual, and we spin up a new upgrade pod that basically uses the syndesis.yml as reference to understand what should be upgraded. If we remove e.g. the todo-app from syndesis.yml, I don't know what happens when we do an upgrade. Best thing I can imagine is that the todo-app won't be upgraded (if we change something in the definition). Worst thing is that it won't be recreated. Unless we fix also the upgrade process to deal with addons...

I understand this concern & don't know enough to solve or reassure at this stage. I do just want to point out that the operator code does have provision for dealing with addons during upgrades -> here. This may help in alleviating the issue but I'll leave that to those more expert than I.

@nicolaferraro
Copy link
Contributor

It's not a problem of expertise, I think there's no common strategy around upgrades with operators, especially for stateful resources. So we need to figure out what's best for us.

If we agree that upgrades should be managed completely by the operator, I think that what you've outlined will help us to start moving things from the current upgrade pod to the operator. The only thing is that currently we are forced to provide also an alternative that works without operators...

@phantomjinx
Copy link
Contributor Author

@chirino
This PR needs a bit of rebasing given the cleanups you've made to the operator. Given your head has been in it recently, do you want to take it over or are you happy for me to rebase the PR and resubmit?

chirino added a commit to chirino/syndesis that referenced this pull request Jun 26, 2019
chirino added a commit to chirino/syndesis that referenced this pull request Jun 26, 2019
chirino added a commit to chirino/syndesis that referenced this pull request Jun 28, 2019
@heiko-braun
Copy link
Collaborator

@phantomjinx Do you intend to continue with this? Master is free for changes like this

@phantomjinx
Copy link
Contributor Author

No longer required as solved in alternative way.

@phantomjinx phantomjinx deleted the no-demo-data branch July 17, 2019 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/review-requested Use this if you want to have a review. pure-bot will prevent merging if set and no review given pr/wip Mark work-in-progress with this label. pure-bot won't merge a PR if this is set.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants