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

Removing and recreating graphiteDB while upgrading tendrl #590

Merged
merged 3 commits into from
Jan 14, 2019

Conversation

GowthamShanmugam
Copy link
Collaborator

bugzilla: 1658245
tendrl-bug-id: #589

Signed-off-by: GowthamShanmugasundaram gshanmug@redhat.com

bugzilla: 1658245
tendrl-bug-id: Tendrl#589

Signed-off-by: GowthamShanmugasundaram <gshanmug@redhat.com>
@GowthamShanmugam
Copy link
Collaborator Author

@shtripat @nthomas-redhat @dahorak please review

utils.remove_file("/var/lib/graphite-web/graphite.db")
utils.print_message("Initializing graphite DB")
utils.command_exec(
"django-admin migrate "
"--settings=graphite.settings "
"--run-syncdb"
)
Copy link

Choose a reason for hiding this comment

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

We should also properly set the ownership of the graphite.db to apache user and group.
It is true, that execution of tendrl-ansible will fix it also, but since the httpd service is already started here in the next step, it will be safer to correct the ownership immediately when the database is initialized.

Copy link
Member

Choose a reason for hiding this comment

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

I agree to this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

"--settings=graphite.settings "
"--run-syncdb"
)

utils.print_message("Starting httpd service")
utils.start_service("httpd")
Copy link

Choose a reason for hiding this comment

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

I think, that we should consider to start also the carbon-cache service. As it was stopped by this script, it might be good to also start it here.
If it will stay stopped and somebody will accidentally run this script (not as part of the whole update workflow), it will leave the cluster in quite inconsistent state, without clearly visible reason (it will not be immediately clear, that the carbon-cache is stopped).

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to start carbon-cache here

utils.remove_file("/var/lib/graphite-web/graphite.db")
utils.print_message("Initializing graphite DB")
utils.command_exec(
"django-admin migrate "
"--settings=graphite.settings "
"--run-syncdb"
)
Copy link
Member

Choose a reason for hiding this comment

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

I agree to this

"--settings=graphite.settings "
"--run-syncdb"
)

utils.print_message("Starting httpd service")
utils.start_service("httpd")
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to start carbon-cache here

import os


def print_message(message):
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a utility function for printing message :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i saw messages are formatted with \n every where, that why i put print in utils, i think we don't need, i will revert it back

bugzilla: 1658245
tendrl-bug-id: Tendrl#589

Signed-off-by: GowthamShanmugasundaram <gshanmug@redhat.com>
@codecov
Copy link

codecov bot commented Jan 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@694d04d). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #590   +/-   ##
=========================================
  Coverage          ?   43.72%           
=========================================
  Files             ?       42           
  Lines             ?     2381           
  Branches          ?      352           
=========================================
  Hits              ?     1041           
  Misses            ?     1287           
  Partials          ?       53
Impacted Files Coverage Δ
tendrl/monitoring_integration/upgrades/utils.py 0% <0%> (ø)
...nitoring_integration/upgrades/delete_dashboards.py 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 694d04d...3b903f8. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@694d04d). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #590   +/-   ##
=========================================
  Coverage          ?   43.33%           
=========================================
  Files             ?       42           
  Lines             ?     2402           
  Branches          ?      356           
=========================================
  Hits              ?     1041           
  Misses            ?     1308           
  Partials          ?       53
Impacted Files Coverage Δ
tendrl/monitoring_integration/upgrades/utils.py 0% <0%> (ø)
...nitoring_integration/upgrades/delete_dashboards.py 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 694d04d...ebf6c34. Read the comment docs.

dahorak
dahorak previously approved these changes Jan 14, 2019
Copy link

@dahorak dahorak left a comment

Choose a reason for hiding this comment

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

Just one small proposal about splitting owner to separated arguments for user and gropu, otherwise it looks good to me.
There are other problems, related to the whole update script (and to this PR too) - for example, no handling for exceptions - but it is much wider issue and out of scope for PR.

print (ex)


def change_owner(path, owner, recursive=False):
Copy link

Choose a reason for hiding this comment

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

It this function should be "universally" used, it would be probably better, to separate owner to user and group arguments (instead of using one value for both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, i will modify and add exception handler for this

bugzilla: 1658245
tendrl-bug-id: Tendrl#589

Signed-off-by: GowthamShanmugasundaram <gshanmug@redhat.com>
@GowthamShanmugam
Copy link
Collaborator Author

I have created utility function and I put separate try-catch for wherever it required, now one function failure won't stop upgrade execution flow.

@GowthamShanmugam
Copy link
Collaborator Author

@dahorak @shtripat changes done

Copy link
Member

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

LGTM.
Hope its verified already.

@shtripat shtripat merged commit e8798f2 into Tendrl:master Jan 14, 2019
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.

3 participants