docksal / ci-agent

CI agent Docker image for Docksal Sandboxes
MIT License
9 stars 16 forks source link

Add method for preserving previous builds during build-init. #48

Closed lpeabody closed 5 years ago

lpeabody commented 5 years ago

Changes

New PR based on discussion from #39.

Summary

This adds functionality for supporting "dirty builds". Dirty builds preserve the build environment and only apply changes to version controlled files and agent-controlled Docksal env variables. Because of the dependency on version control, only Git is supported.

New Environment Variables

Additional Changes

To Do

lpeabody commented 5 years ago

@lmakarov I should note that this is 100% untested as of this moment, and purely theoretical. I wanted to get my thoughts on paper and show something tangible to help facilitate discussion. It was also a good use of my lunch hour...

lpeabody commented 5 years ago

Copying over important discussion from the other thread...

Should build-init just update the codebase and then bail?

What about additional processing currently in build-init which updates docksal-local.env? If we didn't bail and let the rest of the script execute, it would result in duplicate variables being added after every build (not desirable but not necessarily harmful either).

My current thought, though, is it's still best for CI Agent to control the environment variables it's modifying in build-init, and it should update/replace any SECRET variables no matter what in case they were changed/altered after the initial build. I think we could use sed to do the replacement. I have learned a lot about Bash syntax since I've read through fin about a hundred times...

Git is easy, but how to handle rsync?

As far as git goes, what's been working for us is build-exec "git fetch origin && git reset --hard origin/$GIT_BRANCH_NAME".

As far as rsync though, I'm not really an expert as I honestly haven't used that tool in maybe years haha. After a quick Google, and based on how the command is currently structured in build-init, I think all that's necessary is to drop the --delete flag. Needs testing though.

What are your thoughts on the above questions? Are there any questions that should be asked around expected behavior of what remains after a dirty build completes?

lpeabody commented 5 years ago

Also, two questions, first being if REMOTE_BUILD_DIR_CLEANUP is false should it re-initialize or preserve the environment? I read the environment variable name and my first thought is if it's logically true then the project files should be completely removed and re-created (i.e. current functionality). If it is false then it should not remove everything, only update the code that has changed, and update controlled environment variables in docksal-local.env.

Second question, you said false should be the default. Does that mean builds should preserve by default?

lpeabody commented 5 years ago

Also something just occurred to me. I don't think preserve builds works with rsync. What happens if you removed a version controlled file? There isn't really a way for rsync to know to delete that one file since it has no concept of version control. I'll need to experiment with it, but in this very moment I can't really think of a way to do this with rsync.

lpeabody commented 5 years ago

I have tested this on Bitbucket Pipelines with both REMOTE_BUILD_DIR_CLEANUP set to 0 and 1 and it works as you'd expect.

You can test this on your own as well with the image lpeabody/ci-agent:issue-48.1.

lpeabody commented 5 years ago

ping @lmakarov

lpeabody commented 5 years ago

@lmakarov Made some changes according to your feedback. I added additional checking to ensure that REMOTE_CODEBASE_METHOD is valid and that switching from rsync -> git with REMOTE_BUILD_DIR_CLEANUP set to 0 does not cause an error.

For additional context around the error, this is what would happen after a sandbox was previously using rsync as the method, but now trying to preserve builds and use git:

fatal: unable to access 'http://bitbucket.org/genuine/newedelman/': Failed to connect to localhost port 29418: Connection refused

My decision was to exclude .git from being copied over with rsync, so we could check to see if the sandbox was a valid git repo, and if it was then git fetch. If it .git invalid then re-create the environment and clone.

An alternative to excluding the .git directory during an rsync could be, prior to fetching, run something like git remote set-url origin ${GIT_REPO_URL}, I just thought of that actually. Maybe to keep things simple though, we say if you're switching remote codebase methods then a re-initialization is mandatory. Thoughts?

lmakarov commented 5 years ago

Just ran into an undesired consequence of not syncing the top level .git folder via rsync.

Drush in docksal/cli is configured to detect the project root from the git repo root: https://github.com/docksal/service-cli/blob/9a12a7bb4b222ccd94b4017c5e9d0c8997dbb44a/7.1/config/.drush/drushrc.php#L313

This does not work without a repo. As a workaround, running drush within the docroot folder works.

Now, I'm not sure whether relying on a git repo in drush configuration is something we should keep doing or not. I'll see if there are other cases where not having a git repo in the project root causes issues to existing workflows and/or assumptions.

Created a separate ticket for this: https://github.com/docksal/service-cli/issues/123

lmakarov commented 5 years ago

Another thought on this functionality.

In most cases, built-init (which triggers fin init) would result in a complete project stack reset, including the database(s). To fully support persistent environments, init scripts at the project level have to be aware of this mode and act accordingly.

As each project has its own custom init sequence, this cannot be controlled by the ci-agent. All that the agent can do is pass the SANDBOX_PERMANENT variable to the sandbox environment, which it already does.

It can be as simple as this:

if [[ "${SANDBOX_PERMANENT}" == "true" ]]; then
    fin project start
else
    fin project reset -f
fi

@lpeabody are you doing something like that in your project init scripts?

lpeabody commented 5 years ago

Warning: Long and drawn out thoughts below!

TL;DR: We don't use SANDBOX_PERMANENT under any circumstance on our sandbox server so that service-vhost-proxy can reliably cleanup old projects, and leave network space available for new sandboxes. Instead, we use a combination of build-exec and a custom build-preserve script to skirt around the issue of preserving databases in a CI context.

This does not work without a repo. As a workaround, running drush within the docroot folder works.

@lmakarov Yeah that's annoying. I did not hit this because 1) I use Drush aliases liberally and 2) I only tested this against Drush 9, so I don't think drushrc.php was even being used and I don't think I ever issued a Drush command without an alias.

Do you think we should look for another way to switch from rsync to git-based builds without error or is this solved by cleaning up service/cli in a future update?

In most cases, built-init (which triggers fin init) would result in a complete project stack reset, including the database(s). To fully support persistent environments, init scripts at the project level have to be aware of this mode and act accordingly.

So, as far as I can tell build-init does not reset the stack. It initializes the workspace at a file-system level only. However, if you're referring to sandbox-init, that invokes build-init and then assuming that went well it invokes fin init right afterwards.

For most of our projects we customize the init command with options, like fin init -e ci for example where we tell init that it's initializing a CI environment. That is one reason we don't use sandbox-init, with the other reason being that we use a custom build-preserve command which replaces build-init in pipelines where we wish to optionally preserve the environment if it has already been created (i.e. for pipelines that simply execute database updates and configuration imports after the latest code has been pulled for that branch, ideal for QA workflows so the tester does not lose the data they were working with).

To give you an idea of what these pipelines look like and how we're using them, here's a sample of two custom pipelines from bitbucket-pipelines.yml:

    # Main site pipelines below.
    init-main: # Initializes the project for the first time.
      - step:
          script:
            - source build-env
            - build-init
            - build-exec "fin init -e ci default"
    update-main: # Rebuild assets and run updates (i.e. composer, frontend dependencies, etc.).
      - step:
          script:
            - source build-env
            - $BUILD_DIR/.docksal/scripts/pipelines/build-preserve
            - build-exec "fin exec -T /var/www/vendor/bin/blt source:build --environment=ci"
            - build-exec "fin exec -T /var/www/vendor/bin/blt drupal:update --environment=ci"

init-main is used to completely reset the sandbox, where as update-main "preserves" it by essentially doing what this PR does, and then it simply rebuilds assets and runs drush updb/cim/cr. So, we're able to skirt around the database reset issue by using build-exec at the pipelines-level.

We don't leverage SANDBOX_PERMANENT in either scenario because this would tell service-vhost-proxy to never cleanup the project. Every sandbox would always be up, and it would live forever. Our sandbox server has a strict policy of not using this flag so we don't 1) ever have to worry about having sandboxes not able to be started because the network cap has been reached and 2) we can rely on service-vhost-proxy actually deleting old sandboxes.

So, I guess we follow the convention of: The init script is sacred and it will ALWAYS re-initialize the project to give you a fresh and up-to-date installation. If we need to execute an operation where using init would result in an undesirable end-state then we need a different command or set of commands to achieve this. The above sample of bitbucket-pipelines.yml illustrates that philosophy.

That said, everything is flexible and if using SANDBOX_PERMANENT fits into your organizations sandboxing philosophy, then it fits!

I think a question that arises from this is (and this would be a major departure from current Docksal init philosophy), should the project's custom init script be responsible for reseting the project's containers? But now I'm rambling about unrelated things... I think it's an interesting question that could help optimize init scripts and more firmly plant the init script as "this is intended to completely reset your project, use accordingly".