Add storage support. #18
Conversation
| log("Setting Jenkins home to {}".format(home)) | ||
| unitdata.kv().set("jenkins.home", home) | ||
| # re-import paths to force update | ||
| importlib.reload(paths) |
There was a problem hiding this comment.
I don't understand why this is needed. Where does this branch change global variables defined in the paths module?
Note also that the global variables defined in the paths module are meant to be constants, and should not be changed at run time. If you want something to be variable, than please make it a regular variable that is passed around.
There was a problem hiding this comment.
The previous line (not counting the comment) changes the jenkins.home unitdata variable which is what many of the variables in paths are based on. Jenkins itself then has its home reset if needed with 'configuration.set_home()'. If the reload of paths isn't done then when any subsequent functions run they don't work correctly.
There was a problem hiding this comment.
I'm failing to see how the jenkins.home unitdata variable and the global variables in the paths module are related. Where is the code that connects them?
There was a problem hiding this comment.
That was a change I made also, https://github.com/jenkinsci/jenkins-charm/pull/18/files#diff-51fc11c4cd6e41d328a7e7d4f68cf801R8
The fact that this change is part of the diff just adds to the argument that these shouldn't be globals anymore.
There was a problem hiding this comment.
I switched to moving the JENKINS_HOME rather than modifying the Jenkins config in the updated review. That removes this section.
|
This is based on master, I just checked and it says the branch is up to date. Where there specific lines you see as overlap? |
There was a problem hiding this comment.
I didn't dig too deep yet, but it seems to be that:
-
The logic could be simplified and it could take more advantage of the mechanics of reactive hooks.
-
Some of the logic could be moved from reactive.py to lib/ modules, opening the doors to better unit test coverage.
See the comments inline.
| status_set("maintenance", "Adding Jenkins storage") | ||
| homedir = storage_get()['location'] | ||
| unitdata.kv().set("storage.dir", homedir) | ||
| set_state("reset.jenkins.storage") |
There was a problem hiding this comment.
Creating a new state and a new unitdata entry seems a bit convoluted and indirect.
Can't we just have something like:
@hook('jenkins-storage-detaching')
def detaching():
status_set("maintenance", "Adding Jenkins storage")
storage = Storage()
storage.attach()Where the "storage.attach()" call would perform more or less the same logic performed by the "reset_jenkins_storage" in this file.
The detaching hook could be implemented similarly.
There was a problem hiding this comment.
Yes the unitdata entry was an artifact from my previous code you are right it isn't useful anymore. I'll remove it.
| @@ -15,3 +17,39 @@ | |||
| INITIAL_PASSWORD = os.path.join(SECRETS, "initialAdminPassword") | |||
| LAST_EXEC = os.path.join(HOME, "jenkins.install.InstallUtil.lastExecVersion") | |||
| LEGACY_BOOTSTRAP_FLAG = os.path.join(HOME, "config.bootstrapped") | |||
|
|
|||
|
|
|||
| class Storage(object): | |||
There was a problem hiding this comment.
It feels this class should live in its own lib/charms/layer/jenkins/storage.py module. This module should be just be concerned with hard-wired path constants.
| remove_state("jenkins.bootstrapped") | ||
| bootstrap_jenkins() | ||
| else: | ||
| status_set('active', 'Ready') |
There was a problem hiding this comment.
I'm a bit confused by this last bit. Can you clarify why this last bit of logic (the if/else block) is needed at all?
There was a problem hiding this comment.
No need to trigger a bootstrap if it hasn't been done yet, the next Juju hook should trigger it, that is the else case. The status_set was simply to leave this hook in the default state again rather than have it still say, 'Configuring Jenkins Storage'
The if state is because bootstrap needs to be re-triggered to correctly setup Jenkins if had previously been run. This will be encountered if you add storage to an already running application.
There was a problem hiding this comment.
Hm, I now realize the issue a bit better. Essentially from the user perspective the Jenkins deployment would be reset from scratch in the case of "if get_state('jenkins.bootstrapped')", as we discussed on IRC, if I recall correctly.
I'm starting to think that if you don't have a concrete use case for attaching storage after initial deployment, I'd prefer that we ignore the attach/detach storage events completely for now, and perhaps set the status to some error code (e.g. "Attaching storage after deployment not supported").
Alternatively, I'd tend to change my mind and say that we should copy existing data to the new directory (and do that in a separate PR). Otherwise, the behavior is a bit surprising: as I user I prefer to be told that some action is required rather than having my Jenkins apparently reset (I'd have to know that the data has been saved in .bak).
Sorry for the back and forth, and feel free to ping me again on IRC.
|
|
||
|
|
||
| @when("reset.jenkins.storage") | ||
| def reset_jenkins_storage(): |
There was a problem hiding this comment.
I believe most of the logic in this function could be moved to the Storage class, so you can have unit tests coverage for it.
There was a problem hiding this comment.
Perhaps I'm missing something but I'm not sure how the unit tests will work with the remove_state and get_state calls and regardless since those are reactive states they seem like they should be in reactive/jenkins.py
|
|
||
| jenkins_installed = get_state("apt.installed.jenkins") | ||
| if jenkins_installed: | ||
| service_stop('jenkins') |
There was a problem hiding this comment.
Can't we avoid this check by requiring this reactive state hook to be fired only once jenkins is installed?
For example
@when("jenkins-storage-attached", "apt.installed.jenkins")
def attach():
# ...There was a problem hiding this comment.
Yes you could but I realized that in the case where storage was specified on the charm deploy this hook will fire before installing Jenkins. As it is now when Jenkins installs it populates the storage directory immediately and is ready for use. If you wait until after Jenkins is installed you have to restart Jenkins to repopulate the dir and possibly have to redo bootstrapping. It is cleaner to do it correctly from the beginning rather than watching the install restart Jenkins repeatedly and repeat steps for each dir so I thought it worth putting in this if statement.
There was a problem hiding this comment.
Indeed I think this issue is related to how we (don't) handle data migration in case storage is attached post-deployment. See also my reply to the other comment below.
If we had support for transparent data migration I think your concerns would be mitigated. It's true that we'd have to "unnecessarily" copy over a few files (that should be real fast) and restart the service, but this sounds a negligible cost from the user perspective (probably a few seconds altogether on regular hardware).
The benefit would be that we would consolidate code to have a single flow and simplify testing and maintenance. In these cases, I'd tend to prefer to pay the cost of a little overhead and get simpler code in return.
There was a problem hiding this comment.
And for clarity, in case we copy the data over we could still use the symlink, so there would be no need to rewrite files in /etc/default or anything.
| remove_state("jenkins.bootstrapped") | ||
| bootstrap_jenkins() | ||
| else: | ||
| status_set('active', 'Ready') |
There was a problem hiding this comment.
Hm, I now realize the issue a bit better. Essentially from the user perspective the Jenkins deployment would be reset from scratch in the case of "if get_state('jenkins.bootstrapped')", as we discussed on IRC, if I recall correctly.
I'm starting to think that if you don't have a concrete use case for attaching storage after initial deployment, I'd prefer that we ignore the attach/detach storage events completely for now, and perhaps set the status to some error code (e.g. "Attaching storage after deployment not supported").
Alternatively, I'd tend to change my mind and say that we should copy existing data to the new directory (and do that in a separate PR). Otherwise, the behavior is a bit surprising: as I user I prefer to be told that some action is required rather than having my Jenkins apparently reset (I'd have to know that the data has been saved in .bak).
Sorry for the back and forth, and feel free to ping me again on IRC.
|
|
||
| jenkins_installed = get_state("apt.installed.jenkins") | ||
| if jenkins_installed: | ||
| service_stop('jenkins') |
There was a problem hiding this comment.
Indeed I think this issue is related to how we (don't) handle data migration in case storage is attached post-deployment. See also my reply to the other comment below.
If we had support for transparent data migration I think your concerns would be mitigated. It's true that we'd have to "unnecessarily" copy over a few files (that should be real fast) and restart the service, but this sounds a negligible cost from the user perspective (probably a few seconds altogether on regular hardware).
The benefit would be that we would consolidate code to have a single flow and simplify testing and maintenance. In these cases, I'd tend to prefer to pay the cost of a little overhead and get simpler code in return.
|
Transparent data migration is a lot harder then it sounds because one of the reasons for adding storage is because it isn't a few files but a multi-gigabyte footprint. Also on application removal the storage is detached so you have to figure out the difference between a true detach of storage and just wanting to shut it all down. You are right that I am not notifying the user at all that their data is on disk and you can manually sync, there is no real great mechanism to do so. Also though I can think of reasons to add storage after deployment I don't personally need that. Considering those I could drop support for adding storage after installation. Regardless detaching is still needed because that can trigger as part of destroying an application. However even dropping post-deploy storage support isn't quite so simple by the time the charm gets the storage attach hook Juju has already allocated and attached the storage to the vm. If I fail at that point I have just put the charm into an unrecoverable failure state. If I just log how is the user to know that they now have storage attached that is a waste? The best is probably to fail, force the user to run I prefer just better docs on the limitations to attaching storage to a deployed application. |
|
I updated the code to more smoothly handle upgrading of the charm from a non-storage version to a storage version. Also note a new version of juju-deployer with storage support is now available in pypi so only amulet still needs to be built from source to support storage in the amulet tests. |
|
Has anyone tried to use the updated charm without storage? I'm not succeeding in that. #34 |
The latest committed version of amulet and juju-deployer are needed for storage support in the tests.