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

Instantiate Container Template #10737

Merged
merged 1 commit into from
Jan 17, 2017
Merged

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Aug 24, 2016

This PR is built on #10159 - Persist Container Templates.
Changes included:

  • Process a Container Template.
  • Create objects from Container Template.
  • Rollback feature in case of error.
  • Create new project.

@zakiva zakiva changed the title Instantiate template [WIP] Instantiate Container Template Aug 24, 2016
@zakiva
Copy link
Contributor Author

zakiva commented Aug 24, 2016

@simon3z @zeari @moolitayer Please review


ext_management_system.with_provider_connection do |client|
kubeclient = Marshal.load(Marshal.dump(client))
kubeclient.api_endpoint.path = "/api"

Choose a reason for hiding this comment

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

This is somewhat hacky since it has assumptions on kubeclient internal state.
you can use the ext_management_system method that does not use a block to get two clients and do the clean up yourself.

also see this

In the future it would be helpful if we could provide a forwarding client, just like the oc client lets us create objects in OpenShift and k8s alike cc @abonas @simon3z.

@moolitayer
Copy link

Looks good overall , container_template.rb needs some attention

temp[:metadata][:name] = name
temp[:metadata][:namespace] = container_project.name
temp[:objects] = objects
temp[:parameters] = params

Choose a reason for hiding this comment

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

container_template_parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we would like to allow the user to edit the template parameters and not just use them as they are (to be added later on)

Copy link

Choose a reason for hiding this comment

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

its up to you but this might be nicer as:

{
  :metadata => {
    :name      => name
    :namespace => container_project.name
  }

  :objects    => object
  :parameters => params

}

@zakiva
Copy link
Contributor Author

zakiva commented Aug 28, 2016

@moolitayer thanks, I've made the changes please take another look

begin
kubeclient.send("create_#{obj[:kind].underscore}", obj)
rescue KubeException => e
$log.error("Unexpected Exception during creating object: #{e}")
Copy link

@zeari zeari Aug 29, 2016

Choose a reason for hiding this comment

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

  1. s/during/while/g
  2. It doesn't bother me a lot but I wonder if the code here could be smarter:
    • Can we know beforehand if the template should be processed through openshift or kubernetes clients?
    • Should we try the other client if the KubeException is something like a connection timeout?

@simon3z

@simon3z
Copy link
Contributor

simon3z commented Aug 31, 2016

@miq-bot assign zakiva

@miq-bot miq-bot assigned zakiva and unassigned simon3z Aug 31, 2016
@zakiva zakiva force-pushed the instantiate_template branch 2 times, most recently from 478d908 to b483b2d Compare September 6, 2016 15:07
@miq-bot
Copy link
Member

miq-bot commented Sep 14, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@zakiva zakiva force-pushed the instantiate_template branch 3 times, most recently from 941daba to ed67fcb Compare September 20, 2016 11:35
obj = obj.symbolize_keys
obj[:metadata][:namespace] = project
create_entity = "create_#{obj[:kind].underscore}"
client = openshift_client.respond_to?(create_entity) ? openshift_client : kubeclient

Choose a reason for hiding this comment

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

if you are creating objects from different api groups you would have to create a new kubeclient for that group.
Knowing what your target group is available through the apiVersion of the object:

apiVersion: batch/v1
kind: Job

@zakiva zakiva force-pushed the instantiate_template branch 2 times, most recently from 3a9cfde to 6e317ab Compare September 27, 2016 16:07
@zakiva zakiva force-pushed the instantiate_template branch 2 times, most recently from e5f44e5 to 5d15fb6 Compare October 10, 2016 09:40
@zakiva
Copy link
Contributor Author

zakiva commented Oct 10, 2016

@moolitayer please review

@zakiva zakiva changed the title [WIP] Instantiate Container Template Instantiate Container Template Oct 10, 2016
@zakiva
Copy link
Contributor Author

zakiva commented Oct 10, 2016

dropping wip cc @simon3z @zeari
@miq-bot remove_label wip

@bzwei
Copy link
Contributor

bzwei commented Jan 3, 2017

@zakiva I think the build-in rollback implementation (choice No. 1) is fine.

@simon3z
Copy link
Contributor

simon3z commented Jan 4, 2017

@zakiva @zeari I think we should make the required changes to kubeclient ASAP (since it's a dependency and we want to get that out soon).

@zakiva
Copy link
Contributor Author

zakiva commented Jan 8, 2017

@zakiva In the code I saw you have container_project and project. My understanding is that container_project is stored in VMDB table container_projects. What about project? Does it have one-to-one relationship with the template instantiation? Does it get stored in a VMDB table?

@bzwei Please see my comments in the code.

The create_objects method should return @created_objects array. Each created object contains some identifiers that can be used to find the object in VMDB after it is eventually collected through the refresh process. Since the creation of objects triggers a provider refresh, automate can wait until all objects are found in VMDB. Does this make sense?

@zakiva @zeari I think we should make the required changes to kubeclient ASAP (since it's a dependency and we want to get that out soon).

Please note that not all created objects will necessarily be saved in VMDB because we don't collect all kinds of objects.
create_objects returns an array of the created objects (hashes), unless any exception was raised.
Each object has kind, namespace and name that can be used in order to find it in VMDB, if we generally collect this kind of objects.
The only thing I think is missing is a mapping between openshift/k8s kind to miq kind. @simon3z I can simply add a static mapping of these kinds unless we already have it somewere (didn't find any). Also, What kubeclient changes did you refer to? for my understanding we already have everything we need, please let me know if I'm missing something.

@@ -12,4 +12,55 @@ class ContainerTemplate < ApplicationRecord
serialize :objects, Array

acts_as_miq_taggable

def instantiate(params, project = nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bzwei project is the container project name in which the template will be instantiated. This container project is saved in the VMDB as you mentioed, here we just need to use its name. I can change this var name to project_name, it would probably be more accurate.

@@ -23,4 +23,10 @@ def self.event_monitor_class
def supported_auth_attributes
%w(userid password auth_key)
end

def create_project(project)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bzwei project here is a hash that represents the project to be created in openshift and then collected and saved in VMDB table container_projects.

@simon3z
Copy link
Contributor

simon3z commented Jan 9, 2017

Also, What kubeclient changes did you refer to? for my understanding we already have everything we need, please let me know if I'm missing something.

@zakiva I explained to @zeari what we need and why, and I gave him a demo on bluejeans (generateName).

@zeari
Copy link

zeari commented Jan 9, 2017

I explained to @zeari what we need and why, and I gave him a demo on bluejeans (generateName).

@simon3z We get the generate name from process_template and we are able to find the entity using that info so we might not need the kubeclient changes. (correct me if im wrong @zakiva )

@simon3z
Copy link
Contributor

simon3z commented Jan 9, 2017

@simon3z We get the generate name from process_template and we are able to find the entity using that info so we might not need the kubeclient changes. (correct me if im wrong @zakiva )

@zeari we tested that together last time:

oc process -n openshift -v HAWKULAR_METRICS_HOSTNAME=example.com \
    metrics-deployer-template | jq -C .items[].metadata
{
  "generateName": "metrics-deployer-",
  "labels": {
    "component": "deployer",
    "metrics-infra": "deployer",
    "provider": "openshift"
  }
}

Out of the process command we see the defined generateName but not the name of the entity which is generated at creation time.

@zakiva
Copy link
Contributor Author

zakiva commented Jan 9, 2017

@zeari we tested that together last time:

oc process -n openshift -v HAWKULAR_METRICS_HOSTNAME=example.com
metrics-deployer-template | jq -C .items[].metadata
{
"generateName": "metrics-deployer-",
"labels": {
"component": "deployer",
"metrics-infra": "deployer",
"provider": "openshift"
}
}
Out of the process command we see the defined generateName but not the name of the entity which is generated at creation time.

@simon3z @zeari that's correct of course, but we do have the actual name in @created_objects. Kubeclient already returns openshift response which conatins this field. So we can return this array to the automation side, only thing that I see here missing is the kind mapping (as said before)?

@zakiva zakiva force-pushed the instantiate_template branch 2 times, most recently from e110b86 to 2130627 Compare January 10, 2017 09:55
@@ -12,4 +12,69 @@ class ContainerTemplate < ApplicationRecord
serialize :objects, Array

acts_as_miq_taggable

MIQ_ENTITY_MAPPING = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simon3z I added this mapping (only for objects we collect & can instantiate) so the automation side will be able to know in which table to look for the objects.
We may consider adding a more general mapping to reuse it later on in different places.
cc @moolitayer

@@ -23,4 +23,10 @@ def self.event_monitor_class
def supported_auth_attributes
%w(userid password auth_key)
end

def create_project(project)
connect.create_project_request project
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer connect.create_project_request(project)

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

@bzwei
Copy link
Contributor

bzwei commented Jan 12, 2017

With current implementation it should provide sufficient information to start the automate provisioning design/implementation 👍

@miq-bot
Copy link
Member

miq-bot commented Jan 17, 2017

Checked commit zakiva@37e6ece with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0
4 files checked, 0 offenses detected
Everything looks good. 👍

@gmcculloug gmcculloug merged commit 054ac9c into ManageIQ:master Jan 17, 2017
@gmcculloug gmcculloug added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 17, 2017
@simaishi
Copy link
Contributor

@zakiva is there a BZ for this? If not, please create one. Thanks!

@simon3z
Copy link
Contributor

simon3z commented Jan 18, 2017

@bzwei @gmcculloug @zakiva my impression is that we don't need this in euwe. Is that right? (So we don't need to create a BZ as asked by @simaishi and we can drop the euwe label).

@simon3z
Copy link
Contributor

simon3z commented Jan 18, 2017

@zakiva please send another PR adding a comment to rollback_objects stating that the rollback cannot catch children objects created during the instantiation and therefore those objects will remain around.

@bzwei
Copy link
Contributor

bzwei commented Jan 18, 2017

@simaishi This is for future release together with the automate provisioning work (to be developed). Like @simon3z said, it should be dropped from euwe.

@simaishi
Copy link
Contributor

@simon3z @bzwei thank you, changed to euwe/no

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.