Re: Review Request 72728: Added unit tests for the CSI server.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72728/#review221681
---



Bad patch!

Reviews applied: [72728, 72761, 72726, 72754, 72753, 72734, 72733, 72690, 
72716, 72732]

Failed command: cd .. && /usr/bin/python3 support/apply-reviews.py -n -r 72728

Error:
2020-08-21 23:45:17 URL:https://reviews.apache.org/r/72728/diff/raw/ 
[17182/17182] -> "72728.patch" [1]

*** Please tell me who you are.

Run

  git config --global user.email "y...@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: empty ident name (for ) not allowed

Full log: 
https://ci-builds.apache.org/job/Mesos/job/Mesos-Reviewbot-Linux/23/console

- Mesos Reviewbot


On Aug. 21, 2020, 4:34 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72728/
> ---
> 
> (Updated Aug. 21, 2020, 4:34 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit tests for the CSI server.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b 
>   src/tests/csi_server_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72728/diff/9/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72779: Initialized plugins lazily in the CSI server.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72779/
---

(Updated Aug. 21, 2020, 10:06 p.m.)


Review request for mesos and Qian Zhang.


Bugs: MESOS-10163
https://issues.apache.org/jira/browse/MESOS-10163


Repository: mesos


Description
---

Initialized plugins lazily in the CSI server.


Diffs
-

  src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 
  src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a 


Diff: https://reviews.apache.org/r/72779/diff/4/


Testing
---

`sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"`


Thanks,

Greg Mann

Re: Review Request 72779: Initialized plugins lazily in the CSI server.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72779/
---

(Updated Aug. 21, 2020, 10:05 p.m.)


Review request for mesos and Qian Zhang.


Repository: mesos


Description
---

Initialized plugins lazily in the CSI server.


Diffs
-

  src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 
  src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a 


Diff: https://reviews.apache.org/r/72779/diff/4/


Testing
---

`sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"`


Thanks,

Greg Mann

Re: Review Request 72788: Introduced the `CSIPluginInfo.target_path_exists` field.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72788/#review221679
---


Ship it!




Ship It!

- Greg Mann


On Aug. 21, 2020, 2:37 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72788/
> ---
> 
> (Updated Aug. 21, 2020, 2:37 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10150
> https://issues.apache.org/jira/browse/MESOS-10150
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced the `CSIPluginInfo.target_path_exists` field.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 661f746dda9c1d2350867bd9139e73c3e3a34f5d 
>   include/mesos/v1/mesos.proto ffe45c305ab46c73f0da5908287aa3eab8ecf5dd 
>   src/csi/v1_volume_manager.cpp c05265cd514de745e824c16a0acef276deed1510 
> 
> 
> Diff: https://reviews.apache.org/r/72788/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72789: Checkpointed CSI volume state in stringified JSON.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72789/#review221678
---




src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
Line 201 (original), 201 (patched)


Can we do `state::read(volumesPath)` here so that we don't need 
to do any extra JSON/protobuf parsing? Then I think this change may not be 
necessary?


- Greg Mann


On Aug. 20, 2020, 9:16 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72789/
> ---
> 
> (Updated Aug. 20, 2020, 9:16 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10153
> https://issues.apache.org/jira/browse/MESOS-10153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Checkpointed CSI volume state in stringified JSON.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 
> 535974bc9ef4049551d7ea7b464bd000deccd7e8 
> 
> 
> Diff: https://reviews.apache.org/r/72789/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72788: Introduced the `CSIPluginInfo.target_path_exists` field.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72788/#review221677
---



Bad patch!

Reviews applied: [72788]

Failed command: cd .. && /usr/bin/python3 support/apply-reviews.py -n -r 72788

Error:
2020-08-21 16:09:15 URL:https://reviews.apache.org/r/72788/diff/raw/ 
[4332/4332] -> "72788.patch" [1]

*** Please tell me who you are.

Run

  git config --global user.email "y...@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: empty ident name (for ) not allowed

Full log: 
https://ci-builds.apache.org/job/Mesos/job/Mesos-Reviewbot-Linux/15/console

- Mesos Reviewbot


On Aug. 21, 2020, 2:37 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72788/
> ---
> 
> (Updated Aug. 21, 2020, 2:37 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10150
> https://issues.apache.org/jira/browse/MESOS-10150
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced the `CSIPluginInfo.target_path_exists` field.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 661f746dda9c1d2350867bd9139e73c3e3a34f5d 
>   include/mesos/v1/mesos.proto ffe45c305ab46c73f0da5908287aa3eab8ecf5dd 
>   src/csi/v1_volume_manager.cpp c05265cd514de745e824c16a0acef276deed1510 
> 
> 
> Diff: https://reviews.apache.org/r/72788/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72745: Added protobuf messages for offer constraints on a string equality.

[Andrei Sekretenko]

> On Aug. 20, 2020, 7:28 p.m., Benjamin Mahler wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 279-285 (original), 279-317 (patched)
> > 
> >
> > Ok, after the discussion on slack / reviewboard / and your comment here 
> > in the diff, I have a good grasp on the approach and it makes sense.
> > 
> > I think we can get away with a simplified naming scheme and logical 
> > explanation centered around: we only support text, non text is not 
> > supported and will always be matched to let the scheduler do its own 
> > filtering.
> > 
> > I took a stab at the simplified API naming:
> > 
> > ```
> > // Predicates for (pseudo)attribute value equality.
> > //
> > // We currently only support TEXT (string for pseudoattributes)
> > // equality, so these predicates will always match (yield `true`)
> > // for SCALAR or RANGES based attributes. This way, schedulers
> > // will still receive offers for SCALAR and RANGES attributes where
> > // a string equality constraint was specified to mesos, and the
> > // scheduler's own constraint filtering will determine how to
> > // filter these other types of attributes.
> > //
> > // For example: if a scheduler sets a constraint
> > // 
> > //   { "selector": {"attribute_name": "foo"},
> > // "predicate": {"equals":"2.0"} }
> > //
> > // Agents with a SCALAR attribute {"name": "foo", "scalar": 
> > {"value": 1.0}}
> > // will match (since it's SCALAR, not TEXT!) and the scheduler side
> > // filtering will need to filter it according to its desired 
> > semantics.
> > //
> > // Therefore, it is strongly recommended to only use TEXT attributes
> > // on agents. For users with existing attributes, this can be done
> > // by adding a variant of the existing attribute that uses TEXT 
> > instead:
> > //
> > //   --attributes=foo2:v1.0
> > //   which gets parsed into
> > //   {"name": "foo2", "text": {"value": "v1.0"}}, etc.
> > 
> > // Yields `true` if the (pseudo)attribute exists and has a value
> > // equal to this value.
> > //
> > // Non-TEXT (string for pseudoattributes) attributes will always
> > // yield `true` for the reason explained above.
> > message Equals {
> >   required string value = 1;
> > }
> > 
> > // Yields `true` if the (pseudo)attribute does not exist or has
> > // a value not equal to this value.
> > // 
> > // Non-TEXT (string for pseudoattributes) attributes will always
> > // yield `true` for the reason explained above.
> > message NotEquals {
> >   required string value = 1;
> > }
> > 
> > oneof predicate {
> >   Exists exists = 1;
> >   NotExists not_exists = 2;
> > 
> >   Equals equals = 3;
> >   NotEquals not_equals = 4;
> > }
> > ```
> > 
> > Let me know your thoughts!

Dropping the "NonStringOr" part definitely makes sense (and a good explanation 
with examples surely helps).

The only thong I don't like about
```
message Equals {
  required string value;
}
...
// Java code
builder.getPredicateBuilder().getEqualsBuilder().setValue("v1.0")
```
is that it does not give any hint to the reader of the code that the case when 
the attribute is *not even a string* is handled in some special way.

What I'm thinking about is something that would at the very least make the 
person reading the code wonder what happens to non-string attributes, like
```
message StringEquals {
  required string value;
}
...
// Java code
builder.getPredicateBuilder().getStringEqualsBuilder().setValue("v1.0")
```
or, maybe (IMO worse)
```
message Equals {
  required string text;
}
...
// Java code
builder.getPredicateBuilder().getEqualsBuilder().setText("v1.0")
```


- Andrei


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/#review221663
---


On Aug. 20, 2020, 9:09 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72745/
> ---
> 
> (Updated Aug. 20, 2020, 9:09 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10172
> https://issues.apache.org/jira/browse/MESOS-10172
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds protobuf messages for setting offer constraints
> on equality/non-equality of agent's (pseudo)attribute to a specified
> 

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

> On Aug. 20, 2020, 11:33 p.m., Benjamin Mahler wrote:
> > Could you split this into two logical changes?
> > 
> > 1. Add the regex support w/o any caching
> > 2. Add caching of constructed regexes as a memory / performance optimization
> > 
> > It seems like the caching in itself warrants some thought, and so it would 
> > be easier to review them in isolation.

Makes sense; will do that.


> On Aug. 20, 2020, 11:33 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 47-49 (patched)
> > 
> >
> > Some documentation on these and their values would be helpful.
> > 
> > Wow, max_mem is rather complicated, and it looks hard to be confident 
> > about our choice of value:
> > 
> > https://github.com/google/re2/blob/2020-08-01/re2/re2.h#L591-L617
> > 
> > Although that reads as though there is some caching / flexibility on 
> > the memory usage, I assume there is also a hard error if it can't construct 
> > the appropriately initial data structures?
> > 
> > Given this, it's a little scary to not have a tunable flag for it, 
> > should we run into surprising results in practice.
> > 
> > Similarly, for max program size of 100, it's really hard to see how 
> > this value is produced, and while 100 seems like a standard upper bound 
> > (according to envoy docs), it's a little scary to not have a tunable flag 
> > for this one too.
> > 
> > Thoughts?

Yes, there is a hard error if RE2 cannot be constructed without breaching the 
limit.

Not having max_mem / program size as configurable flags indeed seems scary, 
I'll change that.
 
My initial thinking was that having these configurable makes it more complex 
for the frameworks to validate the regexps on their side (so that the 
frameworks can avoid failing subscription/update).
With those limits being configurable, frameworks will need to somehow know the 
exact values of these limits (or Mesos will have to provide a validation API).

Probably you are right, and having those configurable right from the start is a 
lesser evil.


> On Aug. 20, 2020, 11:33 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 60-62 (patched)
> > 
> >
> > Thinking out loud here:
> > 
> > It's really hard for me to tell from here, but the weak cache will 
> > cover the typical path of a scheduler just updating to the same constraint 
> > info as before, right?
> > 
> > Caching wise, it seems most important to (1) avoid the duplication of 
> > regexes within a scheduler, and (2) secondarily avoid the duplication of 
> > regexes across time and across schedulers.
> > 
> > I can see how this covers the second case, since there will clearly be 
> > other references to regexes used by other schedulers. But the first case is 
> > a little less clear because it relies on the caller further up the stack to 
> > not throw away the old constraints object before making the new one, right? 
> > Is that ensured because the allocator will hold a copy, and the master 
> > constructs the new constraints before giving to the allocator to overwrite 
> > the old? This seems like a lot of non-local reasoning needed.
> > 
> > Just makes me wonder if there's a more direct way to cache (where the 
> > caching behavior is clear locally, with less assumption about the caller 
> > baked in). We could consider caching only across a single scheduler's 
> > updates if that's very simple to do and reason about.

Ideally, caching of RE2s should accomplish two goals:
1. Minimize the memory footprint of the used regexps.
2. Avoid re-creating the same regex while keeping the excess memory usage low.

Caching of weak references serves the first goal; it guarantees the minimum 
possible total memory usage of RE2s.
I'm not sure if it is reasonable to assume that different schedulers will use 
different regexps, especially the complex ones.
I can easily imagine a complex regexp selecting 97 agents out of 500 written 
down somewhere on the operator's internal Wiki and copy-pasted from one 
scheduler to another.

On the other hand, the second goal is always a kind of CPU/memory tradeoff.
I fully agree that the fact that weak reference caching also helps with this 
one looks like a coincidence.

Worse, some frameworks in some situations (for example, Marathon with a 
crashlooping app) will find themselves constantly switching a constraint (or, 
more likely, a whole constraint group) on/off. 

Weak caching does not help at all in this case; this is why I'm using an LRU, 
but probably it is worth looking for something better than a global LRU with an 
arbitrary fixed size of 1024.

I would expect that the constraints are extremely unlikely to "jump" from one 
framework to another, so this indeed can b

Re: Review Request 72788: Introduced the `CSIPluginInfo.target_path_exists` field.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72788/
---

(Updated Aug. 21, 2020, 10:37 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Changed the solution by introducing a protobuf message field 
`CSIPluginInfo.target_path_exists`.


Summary (updated)
-

Introduced the `CSIPluginInfo.target_path_exists` field.


Bugs: MESOS-10150
https://issues.apache.org/jira/browse/MESOS-10150


Repository: mesos


Description (updated)
---

Introduced the `CSIPluginInfo.target_path_exists` field.


Diffs (updated)
-

  include/mesos/mesos.proto 661f746dda9c1d2350867bd9139e73c3e3a34f5d 
  include/mesos/v1/mesos.proto ffe45c305ab46c73f0da5908287aa3eab8ecf5dd 
  src/csi/v1_volume_manager.cpp c05265cd514de745e824c16a0acef276deed1510 


Diff: https://reviews.apache.org/r/72788/diff/2/

Changes: https://reviews.apache.org/r/72788/diff/1-2/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 72779: Initialized plugins lazily in the CSI server.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72779/#review221674
---



Bad patch!

Reviews applied: [72779, 72728, 72761, 72726, 72754, 72753, 72734, 72733, 
72690, 72716, 72732]

Failed command: cd .. && /usr/bin/python3 support/apply-reviews.py -n -r 72726

Error:
2020-08-21 09:35:01 URL:https://reviews.apache.org/r/72726/diff/raw/ 
[1538/1538] -> "72726.patch" [1]

*** Please tell me who you are.

Run

  git config --global user.email "y...@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: empty ident name (for ) not allowed

Full log: 
https://ci-builds.apache.org/job/Mesos/job/Mesos-Reviewbot-Linux/10/console

- Mesos Reviewbot


On Aug. 21, 2020, 7:14 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72779/
> ---
> 
> (Updated Aug. 21, 2020, 7:14 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initialized plugins lazily in the CSI server.
> 
> 
> Diffs
> -
> 
>   src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 
>   src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a 
> 
> 
> Diff: https://reviews.apache.org/r/72779/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72727: Updated the test CSI plugin for CSI server testing.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72727/#review221673
---



Patch looks great!

Reviews applied: [72732, 72716, 72690, 72733, 72734, 72753, 72754, 72726, 72727]

Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/jenkins/buildbot.sh

- Mesos Reviewbot


On Aug. 18, 2020, 9:23 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72727/
> ---
> 
> (Updated Aug. 18, 2020, 9:23 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds additional configuration flags to the
> test CSI plugin which are necessary in order to test
> the agent's CSI server.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 214a3ee4481512b025ff054db6ffe15d476b0264 
> 
> 
> Diff: https://reviews.apache.org/r/72727/diff/4/
> 
> 
> Testing
> ---
> 
> These new flags are used in a subsequent test in this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>