-
Notifications
You must be signed in to change notification settings - Fork 898
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 new class ServiceContainerTemplate. #15429
Conversation
$log.log_hashes(opts) | ||
|
||
params = process_parameters(opts[:parameters]) | ||
@created_objects = container_template.instantiate(params, opts[:container_project_name]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not safe to use an instance variable.
@gmcculloug @lfu Evidently we need to remember created objects between steps in the state machine, and more importantly we are always requested to tell the user what objects are created in the provisioning.
Approach No 2 is the way we handle Ansible provisioning. Although Ansible Tower job is a true object in provider, we never collect jobs through refresh. The job is directly created in VMDB. I see the similarity here. |
aec003d
to
7b4dd4b
Compare
# :dialog option should specify the project name, either an existing project or a new project name | ||
dialog_options = options[:dialog] | ||
existing = overrides.delete(:existing_project_name) || dialog_options['dialog_existing_project_name'] | ||
new = overrides.delete(:new_project_name) || dialog_options['dialog_new_project_name'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lfu Avoid using keywords as variable names and would also expect this to be a more descriptive name. Suggest new_project_name
end | ||
|
||
def stack(action) | ||
service_resources.find_by(:name => action, :resource_type => 'OrchestrationStack').try(:resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, we don't need to distinguish action
. There is only one stack.
@bzwei I am ok with the stack approach here, is there a container object this would normally be representing? What type of objects/data would you normally expect to represent as part of the stack? |
|
||
def check_completed(action) | ||
done = stack(action).provider_objects.all? { |obj| obj.resource_category.constantize.find_by(:name => obj.name) } | ||
message = done ? "completed" : "not completed yet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have another message string we use besides "not completed yet"? I'm thinking "in progress" but we should be consistent here if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no standard incomplete message. So I took "in progress".
837e02b
to
324abeb
Compare
@lfu There are a few items missing from your implementation. I'll work with you offline. |
@gmcculloug Container template may contain objects that would be created during the provisioning. An Orchestration stack is added into the service as an virtual layer to keep track of the created container objects. A list of these objects would be returned by Here is an example of the created container object.
|
Another consideration around using stacks is retirement. The base model includes What would happen if |
@gmcculloug Retirement should delete the objects in the provider and then the stack in VMDB |
324abeb
to
51237c3
Compare
end | ||
|
||
def stack | ||
service_resources.find_by(:name => 'Provision', :resource_type => 'OrchestrationStack').try(:resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent do you want to use ResourceAction::PROVISION
?
params | ||
end | ||
|
||
def get_job_options(action) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why job option? there is no concept of job in contain template provisioning.
"#{action.downcase}_job_options".to_sym | ||
end | ||
|
||
def parameters_from_dialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider to make a mix-in to be shared by a few similar service classes.
51237c3
to
8d7902c
Compare
def check_completed(action) | ||
return [true, 'not supported'] unless action == ResourceAction::PROVISION | ||
|
||
done = stack.resources.all? do |obj| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the logic to stack.raw_status
method.
29be82b
to
30d479e
Compare
30d479e
to
990c224
Compare
Travis test failure is due to the dependency on the changes in ManageIQ/manageiq-providers-openshift#27. |
990c224
to
a26e9f7
Compare
@bzwei Can you give this one last review. |
a26e9f7
to
9c87802
Compare
private :add_provider_objects | ||
|
||
def add_provider_object(object) | ||
raise _("Provision with container template [#{container_template.name}] failed: #{object}") if object[:kind].blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not raise an error but mark the resource status failure, and stack status failure?
raise MiqException::MiqOrchestrationProvisionError, err.to_s, err.backtrace | ||
end | ||
|
||
def self.status_class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason the status needs a subclass?
9c87802
to
ebc3a91
Compare
belongs_to :container_template, :foreign_key => :orchestration_template_id, :class_name => "ContainerTemplate" | ||
|
||
def self.create_stack(container_template, params, project_name) | ||
new(:name => container_template.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw the other PR you created OrchestrationStack
for OpenShift and its Status
class. It then makes sense to revert your earlier code to use the Status
subclass, but here you need to create the stack instance with the right class.
miq_class.nil? || miq_obj | ||
end | ||
|
||
update_attributes(:status => 'succeeded') if done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to update_attributes
for the case in line 23.
$log.log_hashes(opts) | ||
|
||
params = process_parameters(opts[:parameters]) | ||
new_stack = ManageIQ::Providers::Openshift::ContainerManager::OrchestrationStack.create_stack(container_template, params, opts[:container_project_name]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derive the full OrchestrationStack
class name from container manager.
ebc3a91
to
e8669a5
Compare
e8669a5
to
b86af5a
Compare
Checked commits lfu/manageiq@0ec3330~...b86af5a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@bzwei @gmcculloug Please review. |
Looks good. |
Add new class ServiceContainerTemplate for container template provisioning.
Includes on ManageIQ/manageiq-providers-openshift#27.
Depends on #15475.
https://www.pivotaltracker.com/story/show/134027413
@miq-bot assign @gmcculloug
@miq-bot add_label enhancement, services, providers/containers
cc @bzwei