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.

Sign up for GitHub

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

Jump to bottom

Archive Container Nodes #15351

Merged
merged 1 commit into from
Sep 1, 2017
Merged

Archive Container Nodes #15351

merged 1 commit into from
Sep 1, 2017

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Jun 12, 2017 •
edited
Loading

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1460401

Depends on ManageIQ/manageiq-schema#22

Changes included:
Adding deleted_on and old_ems_id columns to container_nodes table. (Extracted)

  • Disconnecting container nodes instead of deleting them.
  • Purging archived container nodes.

@zakiva
Copy link
Contributor Author

zakiva commented Jun 12, 2017

@simon3z @zeari Please review

@zakiva
Copy link
Contributor Author

zakiva commented Jun 12, 2017

@miq-bot add_label providers/containers, enhancement

zeari
zeari reviewed Jun 12, 2017
View reviewed changes
app/models/container_node.rb Outdated
self.ext_management_system = nil
self.deleted_on = Time.now.utc
save
end
Copy link

Choose a reason for hiding this comment

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

@simon3z Note we keep entities created by this node like hardware and computer_system. i cant think of a place we might want to exclude them (reasons we would want to implement disconnection for them) but we should keep that in mind

Copy link
Contributor

Choose a reason for hiding this comment

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

@zeari yes we need the hardware (it is important that we keep the number of cpus and memory).

@zakiva zakiva mentioned this pull request Jun 12, 2017
Merged
moolitayer
moolitayer reviewed Jun 19, 2017
View reviewed changes
app/models/container_node.rb
@@ -5,6 +5,8 @@ class ContainerNode < ApplicationRecord
include NewWithTypeStiMixin
include TenantIdentityMixin
include SupportsFeatureMixin
include ArchivedMixin
include_concern 'Purging'
Copy link

@moolitayer moolitayer Jun 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please remove

delegate :my_zone, :to => :ext_management_system

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, thanks

@moolitayer
Copy link

@miq-bot add_label fine/yes

@simon3z
Copy link
Contributor

simon3z commented Jun 19, 2017

@zeari @moolitayer can you review/approve? Thanks.

@moolitayer
Copy link

Had a minor comment, but other then that looks good.
Waiting for next version to give final ok

moolitayer
moolitayer approved these changes Jun 19, 2017
View reviewed changes
Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@zakiva zakiva mentioned this pull request Jun 19, 2017
Closed
@Ladas
Copy link
Contributor

Ladas commented Jun 20, 2017

Btw. it can't be fine/yes. Since we do not allow migrations to be backported,

@simon3z
Copy link
Contributor

simon3z commented Jun 22, 2017

@miq-bot rm_label fine/yes

@miq-bot miq-bot removed the fine/yes label Jun 22, 2017
@simon3z
Copy link
Contributor

simon3z commented Jun 22, 2017

LGTM 👍
ready for merge cc @Fryguy

@miq-bot
Copy link
Member

miq-bot commented Jun 22, 2017

This pull request is not mergeable. Please rebase and repush.

@Fryguy
Copy link
Member

Fryguy commented Jun 22, 2017

Database migrations have now been moved to the https://github.com/ManageIQ/manageiq-schema repo. Please see http://talk.manageiq.org/t/new-split-repo-manageiq-schema/2478 for instructions on how to transfer your database migrations. If this PR contains only migrations, I will leave it open for a short time during the transition, after which I will close this if it has not been moved over.

@zakiva zakiva mentioned this pull request Jun 25, 2017
Merged
@zakiva
Copy link
Contributor Author

zakiva commented Jun 25, 2017

@miq-bot rm_label sql migration

@zakiva zakiva force-pushed the archive_node branch 2 times, most recently from d10703a to 31b34f5 Compare July 11, 2017 14:03
@zakiva
Copy link
Contributor Author

zakiva commented Jul 17, 2017

@Ladas can you please take a look in this PR to see if it's suited to the current state of the new archiving approach?

Ladas
Ladas reviewed Jul 18, 2017
View reviewed changes
app/models/container_node.rb Outdated
@@ -115,4 +114,12 @@ def external_logging_path
index = ".operations.*"
EXTERNAL_LOGGING_PATH % {:index => index, :query => query}
end

def disconnect_inv
return if ems_id.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this can still happen. Maybe the condition should use deleted_on?

Copy link
Contributor Author

@zakiva zakiva Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think you are right. Actually, we might need to change that condition in all other archived containers entities (to avoid second disconnect)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, sounds like a followup PR to clean them all up.

Might be the ems_id.nil? is still relevant when we destroy EMS? At least in cloud/infra, we do dependent => nullify for some models. (to speed up the destroy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in containers we currently have only :dependent => :destroy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ladas fixed it here, will send a separate PR for the rest

Ladas
Ladas approved these changes Jul 18, 2017
View reviewed changes
Copy link
Contributor

@Ladas Ladas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good, I have just 1 minor question

@zakiva zakiva force-pushed the archive_node branch 2 times, most recently from a94c819 to 94d1551 Compare July 24, 2017 08:24
@zakiva
Copy link
Contributor Author

zakiva commented Jul 24, 2017

@Fryguy Please review

@miq-bot
Copy link
Member

miq-bot commented Aug 4, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Aug 9, 2017

Checked commit zakiva@68d0b1f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. 🍰

@cben cben mentioned this pull request Aug 9, 2017
Closed
@zakiva
Copy link
Contributor Author

zakiva commented Aug 15, 2017

@Fryguy @blomquisg Can you please review?

@cben
Copy link
Contributor

cben commented Aug 20, 2017

@moolitayer @Fryguy migration landed long ago, let's get this in.

@moolitayer
Copy link

@Fryguy please review/merge

@cben cben mentioned this pull request Aug 22, 2017
Merged
kbrock
kbrock approved these changes Aug 22, 2017
View reviewed changes
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

LGTM

@zakiva
Copy link
Contributor Author

zakiva commented Aug 27, 2017

cc @chessbyte

@simon3z
Copy link
Contributor

simon3z commented Aug 28, 2017

@moolitayer should we add this to the list of PRs that needs attention? This is waiting since way too long (more than 1 month from first approval and 1 week since last one).
cc @chessbyte @Fryguy @kbrock

@moolitayer
Copy link

I've started a new list with this item in the daily doc.

@Fryguy Fryguy merged commit 042e7f9 into ManageIQ:master Sep 1, 2017
@Fryguy Fryguy added this to the Sprint 68 Ending Sep 4, 2017 milestone Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas approved these changes

@zeari zeari zeari approved these changes

@moolitayer moolitayer moolitayer approved these changes

@cben cben cben approved these changes

@simon3z simon3z simon3z approved these changes

@kbrock kbrock kbrock approved these changes

Assignees

@Fryguy Fryguy

Labels
enhancement providers/containers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@zakiva @moolitayer @simon3z @Ladas @miq-bot @Fryguy @cben @kbrock @zeari