[P4Testgen] Add an option to selectively ignore control plane elements.

[fruffy]

This PR adds an option to skip generating control plane configurations for specified control plane elements. The input is a comma-separated list of the fully-qualified control-plane name. For example, "ingress.acl_table,ingress.fwd_table".

Currently, skipping is only supported for P4 tables, but could be extended to any element. Registers, Meters, Parser Value sets...

[jonathan-dilorenzo]

checkTableImmutability is setting this property based on what's passed in instead? I guess not technically part of this PR, but not a great way to go... Can you just make it 'IsTableImmutable' and then set the property in the same way you do here?

[fruffy]

The current implementation for checkTableImmutability is this one:

void checkTableImmutability(const IR::P4Table &table, TableProperties &properties) {
    bool isConstant = false;
    const auto *entriesAnnotation = table.properties->getProperty("entries");
    if (entriesAnnotation != nullptr) {
        isConstant = entriesAnnotation->isConstant;
    }
    // Also check if the table is invisible to the control plane.
    // This also implies that it cannot be modified.
    properties.tableIsImmutable = isConstant || table.getAnnotation("hidden") != nullptr;
    const auto *defaultAction = table.properties->getProperty("default_action");
    CHECK_NULL(defaultAction);
    properties.defaultIsImmutable = defaultAction->isConstant;
}

It also sets defaultIsImmutable. The main reason the function was constructed this way is because other extensions may pass in an extended TableProperties class. For example, TofinoTableProperties.

The implementation can definitely be improved, I can try to refactor this in a separate PR.

[jonathan-dilorenzo]

Ah, I see. Ok. I would defer to you on how clear the implementation is versus the breadth that it needs to support. Looks pretty fine to me tbh. It just threw me off that they were setting the same value and looked totally different.

[jonathan-dilorenzo]

Maybe you make these manually every time... But would it be worth just having a function that returns something like this for all of your tests?

Regardless, you're probably better off putting this in a function here and then calling it in multiple tests. It would make it more natural to test e.g. what happens if you're given a non-existent control plane entity. Or one that isn't supported yet?

It looks like it would just be ignored effectively. Is that what you want to have happen? Do you want to do greater validation and give an Unimplemented error or an InvalidArgument error depending on the situation?

[fruffy]

Maybe you make these manually every time... But would it be worth just having a function that returns something like this for all of your tests?

Yes! I thought about this, but I figured I should wait until have more clarity on the test patterns, e.g., what elements and headers I keep reusing.

It looks like it would just be ignored effectively. Is that what you want to have happen?

So this was intentional. Mostly because P4Testgen currently does not have a collection of valid control-plane elements. I first would need to create that collection in a target-independent manner (since BMv2 has different control-plane elements to Tofino or the DPDK-target), then perform the lookup. Presumably I could do this with a P4Info, but again not every extension uses that either.

It would be nice to have that validation, but it will require some work.

[jonathan-dilorenzo]

I see. It seems fine to skip the validation (I think that's fairly typical), but I would prefer to have some tests with those cases showing that nothing catastrophic happens at least. I think that's the only change I really would want to see.

And again, I would recommend doing it (at least locally in this file) by factoring out the P4 program creating to a function that you call in ~4 different test cases (showing that it works with 1 or multiple tables, and the behavior with a not-yet-supported and non-existent entity).

[jonathan-dilorenzo]

Should it be clear why you always expect the drop one to be first? Seems like an implementation detail? Probably better to loop through them and check that at least one is a drop and at least one exercises NoAction?

[fruffy]

Tried to implement something here. Any way to use a lambda in Protobuf matchers? Currently I need to convert the entire vector to strings before passing them to the matchers.