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

perf(filestate): DoesProjectExist should not list all #12547

Closed
abhinav opened this issue Mar 29, 2023 · 6 comments · Fixed by #12798
Closed

perf(filestate): DoesProjectExist should not list all #12547

abhinav opened this issue Mar 29, 2023 · 6 comments · Fixed by #12798
Assignees
Labels
area/backends State storage (filestate/httpstate/etc.) good-first-issue Start here if you'd like to start contributing to Pulumi help-wanted We'd love your contributions on this issue impact/performance Something is slower than expected kind/enhancement Improvements or new features resolution/fixed This issue was fixed

Comments

@abhinav
Copy link
Contributor

abhinav commented Mar 29, 2023

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

There's room for a small performance improvement to the DoesProjectExist method
in the filestate backend:
Instead of listing all projects and searching through them, it could search for
the prefix $projectName/, and report success if that doesn't fail with
NotFound.

Credit to @Frassle for the suggestion:
#12437 (comment)

Affected area/feature

filestate backend

@abhinav abhinav added area/backends State storage (filestate/httpstate/etc.) impact/performance Something is slower than expected labels Mar 29, 2023
@justinvp justinvp added the kind/enhancement Improvements or new features label Mar 29, 2023
@abhinav abhinav added the good-first-issue Start here if you'd like to start contributing to Pulumi label Mar 31, 2023
@RobbieMcKinstry RobbieMcKinstry added the help-wanted We'd love your contributions on this issue label Apr 11, 2023
@rafiramadhana
Copy link
Contributor

Hi @abhinav , I am interested to work on this issue.

Here is my proposed solution:

  • Update projectReferenceStore.ListProject (store.go#L207) to accept $projectName as arg (or we can put the value in ctx to preserve backward compatibility)
  • Update prefix in store.go#L208 from path := StacksDir to path := StacksDir + "/" + $projectName

Btw, I am assuming the behavior of listBucket (bucket.go#L66) to support filtering by having $projectName in the prefix (sry, haven't tried to explore the repo that much)

Please LMK if you have concern or anything. Thanks!

@abhinav
Copy link
Contributor Author

abhinav commented Apr 26, 2023

Thanks, @neverbeenthisweeb! A PR would absolutely be welcome.

Adding projectName to ListProjects is a valid option.
However, it may be easier to add a new method to projectReferenceStore.
For example, if we added a ProjectExists(ctx context.Context, projectName string) (bool, error) method to projectReferenceStore, it could implement the specialized behavior, while still keeping the layout details isolated to the projectReferenceStore.

The implementation that you're planning itself sounds reasonable: see if listing path.Join(StacksDir, projectName) returns an empty result.

@rafiramadhana
Copy link
Contributor

@abhinav I see, agree. Would you mind to assign this to me?

@abhinav
Copy link
Contributor Author

abhinav commented Apr 27, 2023

Done!

@rafiramadhana
Copy link
Contributor

rafiramadhana commented Apr 29, 2023

@abhinav Hi, questions

Let's say we have this bucket structure

.
└── pulumi/
    └── stacks/
        ├── a
        └── b/
            └── foo.json

I'm quite sure that we consider project b as exist here, CMIIW.

However, do we consider project a as exist too?

As a context, I am going to use listBucket (https://github.com/pulumi/pulumi/blob/master/pkg/backend/filestate/bucket.go#L66) for checking if the project exist. But It looks like listBucket is not able to differ between non-exist project and empty project.

PS. I've created a draft PR for this case (see https://github.com/pulumi/pulumi/pull/12798/files#r1181096721)

@Frassle
Copy link
Member

Frassle commented Apr 29, 2023

I would say "a" should return as not existing. A project exists if it has any stacks.

bors bot added a commit that referenced this issue May 2, 2023
12798: Update DoesProjectExist to not list all projects r=abhinav a=neverbeenthisweeb

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

<!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. -->

Add `projectReferenceStore.ProjectExists`.

`projectReferenceStore.ProjectExists` enables us to search for a specific project in `localBackend.DoesProjectExist`.

**Tasks**

- [X] Add `projectReferenceStore.ProjectExists`
- [X] Replace `projectReferenceStore.ListProjects` with `projectReferenceStore.ProjectExists` in `localBackend.DoesProjectExist`

Fixes #12547

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [X] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [X] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Cloud,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


Co-authored-by: neverbeenthisweeb <rf.ramadhana@gmail.com>
bors bot added a commit that referenced this issue May 2, 2023
12798: Update DoesProjectExist to not list all projects r=abhinav a=neverbeenthisweeb

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

<!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. -->

Add `projectReferenceStore.ProjectExists`.

`projectReferenceStore.ProjectExists` enables us to search for a specific project in `localBackend.DoesProjectExist`.

**Tasks**

- [X] Add `projectReferenceStore.ProjectExists`
- [X] Replace `projectReferenceStore.ListProjects` with `projectReferenceStore.ProjectExists` in `localBackend.DoesProjectExist`

Fixes #12547

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [X] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [X] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Cloud,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


Co-authored-by: neverbeenthisweeb <rf.ramadhana@gmail.com>
@bors bors bot closed this as completed in f69c473 May 3, 2023
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backends State storage (filestate/httpstate/etc.) good-first-issue Start here if you'd like to start contributing to Pulumi help-wanted We'd love your contributions on this issue impact/performance Something is slower than expected kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants