As we'll need to use folly for the dev mode in the backup as well, I decided to move folly installation from the blob base image to the backup base image
Depends on D3179
Details
Details
Remove backup-base, backup-server, blob-base and blob-server as well as all caches and rebuild the images:
cd services yarn build-backup-base yarn build-blob-base yarn run-blob-service yarn run-backup-service
services should work as they did before this change.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Can you remind me again, does the blob base build on the backup base? @jimpo, what's your suggestion for how to share the build config for this dependency across multiple services?
Passing back to you since I need a bit more context before I can accept
Comment Actions
OK, a related issue here is that I'd prefer folly to be built and installed as a system library instead of pulled into the CMake build of the service. This both helps for Docker layer caching and simplifying the CMakeLists.txt. I don't care as much whether it's in the service image or the base image, but one argument for service image is that we may want to build folly with different features/flags/build parameters for different services.
Comment Actions
Can you remind me again, does the blob base build on the backup base?
Yes.
OK, a related issue here is that I'd prefer folly to be built and installed as a system library instead of pulled into the CMake build of the service. This both helps for Docker layer caching and simplifying the CMakeLists.txt. I don't care as much whether it's in the service image or the base image, but one argument for service image is that we may want to build folly with different features/flags/build parameters for different services.
Can you specify what do you mean exactly? I think folly is installed as a system lib and symlinked to the local dir.
We can refactor services but I think it would be good to do this in a separate diff and get this landed.
| services/backup/docker-server/contents/server/cmake-components/folly.cmake | ||
|---|---|---|
| 12–47 ↗ | (On Diff #9662) | I don't see this list of files anywhere in the before state, and the diff name/description suggest that the diff is just moving the same build config from the backup image to the base image. Are you actually changing the build config here? If so, can you please move split this diff into two – one that moves the build config, and one that updates it? Alternately, if this is getting moved/copied from somewhere, can you please indicate the source? |
| services/backup/docker-server/contents/server/cmake-components/folly.cmake | ||
|---|---|---|
| 12–47 ↗ | (On Diff #9662) | |
This comment was removed by ashoat.
Comment Actions
Please link a task that tracks fixing this very strange/confusing dependency chart (one service depends on another). It feels like we've let you be a bit too laissez-faire with some of the Docker services config... the idea that the blob service would extend the backup service honestly seems quite ridiculous to me. If I was more of an expert in Docker, I probably would've felt more comfortable requesting changes on this in the past. Hopefully @jimpo will help us catch these sorts of issues in the future
| services/backup/docker-server/contents/server/cmake-components/folly.cmake | ||
|---|---|---|
| 12–47 ↗ | (On Diff #9662) | Wait so why aren't we deleting that file, if it's being moved here? Do we need it in both places long-term? Why is that? |
| services/blob/docker-base/Dockerfile | ||
|---|---|---|
| 3 ↗ | (On Diff #9836) | error |
Comment Actions
https://linear.app/comm/issue/ENG-804/fix-services-images
We discussed why we have such a dependency chain a couple of times I think. I see however that it brings more and more confusion, therefore I'm going to push here some changes that get rid of it and an additional diff that follows the DRY rule.
| services/blob/docker-base/contents/install_aws_sdk.sh | ||
|---|---|---|
| 5–9 ↗ | (On Diff #9837) | Pattern of semicolon use here is a bit strange. Looking at existing conventions in the codebase, here are some nits. (If this is copied from somewhere, feel free to fix it in both places in a follow-up diff) |
| 7–8 ↗ | (On Diff #9837) | Please stick with two-char tabs everywhere. (If this is copied from somewhere, feel free to fix it in both places in a follow-up diff) |
| services/blob/docker-base/Dockerfile | ||
|---|---|---|
| 13 ↗ | (On Diff #10011) | In this change, you pull out rm -rf /var/lib/apt/lists/* into its own line. My (limited) understanding is that you are breaking build caching by doing this. If I'm not wrong in my above analysis, it's frankly rather concerning to see you make this change. When making a change like this, you should (in order):
|
| services/blob/docker-base/contents/install_aws_sdk.sh | ||
| 7–8 ↗ | (On Diff #10011) | Two-space tabs |
| services/blob/docker-base/contents/install_aws_sdk.sh | ||
|---|---|---|
| 5–9 ↗ | (On Diff #9837) | Please don't forget to respond to this comment |
Comment Actions
Created a task explaining it https://linear.app/comm/issue/ENG-825/install-folly-as-system-library-in-base-service-image
| services/backup/docker-server/contents/server/cmake-components/grpc.cmake | ||
|---|---|---|
| 11 ↗ | (On Diff #10038) | These flags don't do anything here, they need to be set when gRPC is built, which is in install_grpc.sh. Related to https://linear.app/comm/issue/ENG-824/dont-build-redundant-dependencies-for-grpc. |
Comment Actions
Requesting changes for @jimpo's last comment above. Remember – if you don't request changes / accept when you leave a comment, then the diff stays on your queue instead of going back to the diff author's!
| services/blob/docker-base/contents/install_aws_sdk.sh | ||
|---|---|---|
| 5–9 ↗ | (On Diff #10038) | Please fix the tabs and semicolons here, as discussed in D3268 |
Comment Actions
Eh, not necessary to address here was just pointing it out. I made a note in ENG-824 so we can follow up and fix.
| services/backup/docker-server/contents/server/cmake-components/grpc.cmake | ||
|---|---|---|
| 11 ↗ | (On Diff #10038) | Thanks for spotting this. I think we can cover this in the task you linked. The current task is just refactoring/moving stuff. I wouldn't mix it with fixing the grpc build. |
| services/backup/docker-server/contents/server/cmake-components/folly.cmake | ||
|---|---|---|
| 12–47 ↗ | (On Diff #9662) | Answered in https://phabricator.ashoat.com/D3198#87347 |
| services/backup/docker-server/contents/server/cmake-components/folly.cmake | ||
|---|---|---|
| 12–47 ↗ | (On Diff #9662) | I don't think that answers the question. I'm asking here very speciifcally about the Folly stuff – the discussion you've linked is general. It reminds me of this lyric from a band I like:
It's possible that there is a diff out already addressing this, but you have simply not given me the context to know that (and as a result, I've spent 5 minutes typing this out). In the future, can you please give more precise answers, and track things precisely? If there's a diff: link it. If all you have is a general task that doesn't talk about this issue specifically, then either create a new task or modify the existing task to clarify that it covers this. At the very least, you could add a comment to the task explaining how it relates. |
| services/backup/docker-server/contents/server/cmake-components/folly.cmake | ||
|---|---|---|
| 12–47 ↗ | (On Diff #9662) | What exactly is the problem here?
We have folly.cmake for backup and for blob services. This repetition is tracked by https://linear.app/comm/issue/ENG-324/avoid-repetitions-in-services-code. Additionally, there's the same code in the tunnelbroker's cmake but I don't know at what stage the tunnelbroker is and I don't want to go there and mess it up too much just like that. If there are any more problems or I misunderstood something, please let me know. BTW I can see more and more complaints about these repetitions. We can prioritize the task of getting rid of them. At first, the idea was to first finish the services and then look at the file, see what's mutual and exclude it to the common space. But since it's become a big problem, I can take care of it and get back to the things I'm working on now after that. |
Comment Actions
I have two points of feedback I want to emphasize.
Your tasks are confusing
We have folly.cmake for backup and for blob services. This repetition is tracked by https://linear.app/comm/issue/ENG-324/avoid-repetitions-in-services-code
That task does not mention anything about this. This is what I was getting at above with ENG-804 too... you tell me that these tasks track these issues, but there is nothing in the task to indicate what they are tracking. They are both effectively "generic DRY" tasks.
You should not have generic tasks like this. They should be more precise, with more details linked and more in the description. I frequently find myself having to edit your tasks, add comments for clarification... this shouldn't be necessary. The tasks should be clearly labeled, and you should never have to tell somebody what they are tracking... it should be clear in the task. Right now I can't even tell what the difference is supposed to be between ENG-324 and ENG-804, and neither of them mention anything about Folly.
Besides the tasks, your responses to my questions also feel sort of similar... my original question was about why you're copy-pasting Folly, and you still haven't answered it. I don't understand why this is necessary, and why we can't just have a single Folly config. Until I understand that, you are going to keep getting comments on every diff that touches Folly, because I still have no idea how Folly is configured and I am unable to effectively review your diffs without this knowledge.
When somebody asks a question, you should answer it. If you point them to a task instead, that task should answer the question. It's not okay to deflect a question with a link to a task that doesn't discuss that question at all.
I have no idea what's going on
BTW I can see more and more complaints about these repetitions. We can prioritize the task of getting rid of them. At first, the idea was to first finish the services and then look at the file, see what's mutual and exclude it to the common space. But since it's become a big problem, I can take care of it and get back to the things I'm working on now after that.
I'm actually fine with this prioritization. The reason you keep getting pushback on this stuff is because I have no idea what's going on with the current state. I constantly encounter weird configs in diffs, and when I push back you just tell me it's tracked in some generic task that has like one sentence in its description.
The result is that I don't have any idea of the state of the world – what's currently misconfigured and what needs to be done to fix it. If I had this context, I wouldn't keep needing to ask you about it in every diff. In an ideal world, I understand what is being copy-pasted, why it's being copy-pasted, and how we're planning to make it better. Currently I have none of that, and the result is that both of us have to waste time on every diff.
Comment Actions
This task is about moving mutual files from all services into some mutual space. So what am I supposed to do? List every file that I'm concerned may be re-used between services? If there's the same file in multiple services then it is tracked by this task. If the task is too generic I can mention there every file that is being repeated.
Besides the tasks, your responses to my questions also feel sort of similar... my original question was about why you're copy-pasting Folly, and you still haven't answered it. I don't understand why this is necessary, and why we can't just have a single Folly config. Until I understand that, you are going to keep getting comments on every diff that touches Folly, because I still have no idea how Folly is configured and I am unable to effectively review your diffs without this knowledge.
I don't understand what is the problem. I stated this before and you didn't answer inline.
here https://phabricator.ashoat.com/D3198#92902 I asked specifically:
What exactly is the problem here?
Without me understanding what is the problem, you will not receive a decent answer from me. Is one able to answer a question when they do not know the question itself? I'm highly doubting that.
I'm copy-pasting folly because I need it in two places. And the reason we're not reusing one code yet is because of the prioritization that we've talked about and you've agreed on that firstly we finish the services(up to some point) and then pull the mutual stuff to the common space.
When somebody asks a question, you should answer it. If you point them to a task instead, that task should answer the question. It's not okay to deflect a question with a link to a task that doesn't discuss that question at all.
I asked what exactly is a problem here and you didn't answer either. There's clearly a problem in communication and understanding each other. The sad thing about this is that it escalated from something simple I believe.
The result is that I don't have any idea of the state of the world – what's currently misconfigured and what needs to be done to fix it. If I had this context, I wouldn't keep needing to ask you about it in every diff. In an ideal world, I understand what is being copy-pasted, why it's being copy-pasted, and how we're planning to make it better. Currently I have none of that, and the result is that both of us have to waste time on every diff.
Ok, let's try to fix this up.
How about that: let's end this discussion and leave it behind. Next, please construct clear questions on what you exactly do not understand, ideally in points. Then, I'm going to try to answer all of them. Thanks.
Comment Actions
I don't understand what is the problem. I stated this before and you didn't answer inline.
That's unfortunate to hear, given I have spent at this point probably around 30 minutes trying to articulate precisely what the problem is.
It's clear that we're not making any traction over Phabricator. As I suggested to you last week, let's drop the conversation here and follow up in our 1:1 this week.