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

Add a design document proposal to support open in VU context #3358

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Sep 28, 2023

This draft aims to address #3020, focusing on enabling k6 scripts to access files directly from the VU stage, not just the init stage.

While the primary focus is on file access, I've currently left out considerations regarding require from #3020. If require should be part of this discussion, based on similar requirements or challenges, I'm more than open to updating the proposal to encompass it.

If you've got alternative solutions or insights, please contribute with a separate commit.

I'm eager to hear your thoughts and feedback in the comments below. Thanks for taking the time to review! 🙇🏻

@oleiade oleiade self-assigned this Sep 28, 2023
@oleiade oleiade added the docs label Sep 28, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (6fa8abd) 72.96% compared to head (1e7aba0) 72.96%.

❗ Current head 1e7aba0 differs from pull request most recent head f708952. Consider uploading reports for the commit f708952 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3358   +/-   ##
=======================================
  Coverage   72.96%   72.96%           
=======================================
  Files         261      261           
  Lines       20018    20018           
=======================================
  Hits        14607    14607           
  Misses       4484     4484           
  Partials      927      927           
Flag Coverage Δ
ubuntu 72.90% <ø> (-0.01%) ⬇️
windows 72.82% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

We should add a comparison in terms of pros&cons for the solutions proposed and we should add a section for eventual risks.

I usually prefer coding soltuions but this time I would prefer the option. I think it enables the concept of injecting the infrastructure where the file system could be part of.

Having the option could allow us to control this injection from the CLI, which sounds closer to what ops are already doing, mounting volumes.

I have the feeling that the Option's implementation will be simpler as we don't have to go through the code and scan for this special function.

- Opening an un-included file in the VU stage will result in an error.
Files opened in the init stage without prior inclusion should still operate as they currently do (though this is debatable) to maintain backward compatibility.

#### Using dedicated option set
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#### Using dedicated option set
### Solution B: A dedicated option set

I evaluate it as a different proposal because they should be mutually exclusive. I don't see the reason why we should eventually implement both.


### Solution B: Awaiting Proposals

Have a novel idea? Please share it with us in a dedicated commit :bow:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would argue we do not need a new API at all.

There are two problems:

  1. you can't at all open files outside of init code
  2. it might be beneficial to have an API that tells k6 to allow a given file to be openable outside of the init code.

There is also the sligth twist that actually open will return you an error even in init code if you try to open a file in a VU after the initial one that was not opened by the initial one.

#2314

open("./someotherfile.txt") // will not error out for either the initial nor the VU with id `1`
if (__VU == 1) {
  open("./somefile.txt") // will always error out as the initial VU did not open it.
}

Again the first open is called both in VU with id 0 but also for any VU that will execute a default function (for example). See #1771 for the rational and discussion.

the tl;dr is that if we want to allow open to at all execute in vu code we can have the same limitation - the file needs to be opened in the VU==0. This is one decision and change.

Adding API/configuration that adds files (again in the initial VU as discussed in #1771) is IMO a separate discussion.

@oleiade
Copy link
Member Author

oleiade commented Oct 12, 2023

Heads-up that @codebien volunteered himself to take over this topic, and will be handling the discussions and work on that front from now on 🙇🏻

PS: @szkiba expressed interest in proposing an alternative solution for this too 👍🏻

@oleiade oleiade assigned codebien and unassigned oleiade Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants