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

propose adr 0003 for workspace blob caching #1010

Merged
merged 5 commits into from
Oct 11, 2023

Conversation

chanwit
Copy link
Collaborator

@chanwit chanwit commented Sep 20, 2023

Fixes #972

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>
@chanwit chanwit self-assigned this Sep 20, 2023
yitsushi
yitsushi previously approved these changes Sep 20, 2023
docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
@yitsushi
Copy link
Collaborator

Added some notes, but the doc itself looks good to me.

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>
yitsushi
yitsushi previously approved these changes Sep 20, 2023
Copy link
Contributor

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

This ADR needs to be precise about the problem it addresses and how it addresses it. At present it gives its rationale in terms of adjectives -- "streamlined", "efficient", "optimized" -- but

  1. it's not clear why optimising is important, except in general terms
  2. the claims aren't supported by the explanation of the design

I suggest you revisit the context, and explain what is the "resource deletion problem" and what effect it has for users. Then you can rephrase the explanation of the design in terms of how it breaks down that problem, mechanically.

docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
docs/adr/0003-workspace-blob-caching.md Show resolved Hide resolved
docs/adr/0003-workspace-blob-caching.md Outdated Show resolved Hide resolved
Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>
yitsushi
yitsushi previously approved these changes Oct 9, 2023
@chanwit chanwit force-pushed the adr-0003-workspace-blob-caching branch 3 times, most recently from 0f4f22f to dab47a4 Compare October 9, 2023 13:18
@chanwit chanwit requested review from yitsushi and squaremo October 9, 2023 13:19
Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>
@chanwit chanwit force-pushed the adr-0003-workspace-blob-caching branch from dab47a4 to fac76f9 Compare October 9, 2023 13:24
Copy link
Contributor

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Thanks for adding the explanation of the problem. The text reads better as an argument, now -- here's the problem, we're going to solve it this way, because ...".

I was tripped up by the sentence "If we try to remove [a Terraform object] without deleting these resources [it relies on], the TF object gets stuck in an inconsistent state [...]" -- but I may have misunderstood. Revise it if you can see what I mean in my comment :-)


These problems must be fixed in the above order as (2) and (3) require single object deletion to be resolved first.

Deleting a single TF object can sometimes be obstructed because it's tied to other resources like Source objects, Secrets, and ConfigMaps. If we try to remove it without deleting these resources, the TF object gets stuck in an inconsistent state, making it harder for users to manage their infrastructure smoothly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the solution given below I would expect the problem to be that a Terraform object can't be finalized if resources it depends on are deleted before it is. The description here suggests vice versa: that deleting a Terraform object before the things it relies on leads to the problem. Have I misunderstood?

Deleting a single TF object can sometimes be obstructed because it's tied to other resources like Source objects, Secrets, and ConfigMaps. If we try to remove it without deleting these resources, the TF object gets stuck in an inconsistent state, making it harder for users to manage their infrastructure smoothly.
Therefore, the TF-Controller is being enhanced to address this problem more efficiently, using the contents of generated Workspace BLOBs. Each BLOB contains all necessary information from the associated Source, Secrets, and ConfigMaps to ensure that TF-Controller finalization procedures can delete objects correctly.

Currently, the TF-Controller downloads a Source BLOB and pushes it to a tf-runner. The tf-runner processes this BLOB to create a Workspace file system. It generates a backend configuration file, variable files, and other necessary files for the Workspace file system, using data from associated Secrets and ConfigMaps. This newly created Workspace file system is then compressed, sent back to the TF-Controller, and stored as a Workspace BLOB in the controller's storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

This newly created Workspace file system is then compressed, sent back to the TF-Controller, and stored as a Workspace BLOB in the controller's storage.

This is given as part of the solution below -- "A gRPC function named CreateWorkspaceBlob will be invoked by the TF-Controller to tell tf-runner to compress the Workspace file system into a tar.gz BLOB, which is then retrieved back to the controller". Is it current behaviour, or proposed behaviour?

These problems must be fixed in the above order as (2) and (3) require single object deletion to be resolved first.

Deleting a single TF object can sometimes be obstructed because it's tied to other resources like Source objects, Secrets, and ConfigMaps. If we try to remove it without deleting these resources, the TF object gets stuck in an inconsistent state, making it harder for users to manage their infrastructure smoothly.
Therefore, the TF-Controller is being enhanced to address this problem more efficiently, using the contents of generated Workspace BLOBs. Each BLOB contains all necessary information from the associated Source, Secrets, and ConfigMaps to ensure that TF-Controller finalization procedures can delete objects correctly.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is it more efficient than?

If I'm a user that has got into this situation of having an undeletable Terraform object, is there anything I can do to unstick it -- presumably, recreating the missing resources would work, right? If so, I think you should mention that here or as part of the explanation of the problem; and, say that the proposed solution makes it much less likely that a user will have to step in and fix things themselves. (That really would be more efficient!)

@chanwit chanwit merged commit 712b845 into main Oct 11, 2023
5 checks passed
@chanwit
Copy link
Collaborator Author

chanwit commented Oct 11, 2023

Will do a follow-up PR to address your comments.

@bigkevmcd bigkevmcd deleted the adr-0003-workspace-blob-caching branch December 12, 2023 17:58
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.

Write an ADR for the workspace blob caching
3 participants