Enable set_field with Metadata register#16
Conversation
commandgjj
force-pushed
the
Enable_SetField_Metadata_Register
branch
from
75dcf64 to
1d72301
Compare
|
Maintainers, can you please help review this patch? Thanks! |
commandgjj
force-pushed
the
Enable_SetField_Metadata_Register
branch
from
1d72301 to
9951b72
Compare
@wenyingd Wenying, can you please help review this change? Thanks! |
commandgjj
force-pushed
the
Enable_SetField_Metadata_Register
branch
from
9951b72 to
cc4335d
Compare
commandgjj
force-pushed
the
Enable_SetField_Metadata_Register
branch
from
cc4335d to
781cf31
Compare
ofctrl/fgraphFlow.go
Outdated
|
|
||
| case "setMetadata": | ||
| // Set Metadata instruction | ||
| case ActTypeSetMetadata: |
There was a problem hiding this comment.
Actually, I am a bit confused. I know you want to add support for applyAction set metadata, so you add a new OFAction with it, but here you keep the original write_metadata instruction.
With your current code, if using SetMetadataAction.GetActionMessage, an ApplyAction message is generated, and can be add into ApplyActionInstruction, but if call Flow.installFlowActions it uses write_metadata instruction.
My point is, please keep the behavior compatible, maybe you should rename the original action type as write_meteadata, and add a new type set_metadata, and add call in both SetMetadataAction.GetActionMessage, and Flow.installFlowActions.
There was a problem hiding this comment.
Actually, I am a bit confused. I know you want to add support for applyAction set metadata, so you add a new OFAction with it, but here you keep the original write_metadata instruction.
With your current code, if using
SetMetadataAction.GetActionMessage, an ApplyAction message is generated, and can be add into ApplyActionInstruction, but if callFlow.installFlowActionsit uses write_metadata instruction.My point is, please keep the behavior compatible, maybe you should rename the original action type as write_meteadata, and add a new type set_metadata, and add call in both
SetMetadataAction.GetActionMessage, andFlow.installFlowActions.
OK. I know your point. I renamed the origianl action as "writeMetadata". Please help review it again, thanks @wenyingd .
commandgjj
force-pushed
the
Enable_SetField_Metadata_Register
branch
2 times, most recently
from
4371814 to
63c14da
Compare
WriteMetadata is write-action instruction which will be applied later than apply-action. In some case, we need use resubmit to do pipeline in the same table and set Metadata before resubmit at the same time. WriteMetadata cannot do the job. We need enable set_field for Metadata register. Actually, WriteMetadata do the same thing with set_field Metadata register in the OvS. From OpenFlow 1.5, Metadata register also can be used with set_field to set, not stricted only using WriteMetadata instruction. After this patch, ofnet can support WriteMetadata and set_field for Metadata meanwhile. Caller can use SetMetadata() or WriteMetadata() API to implement WriteMetadata action, and use SetMetadataAction() and ApplyAction() to implement set_field for Metadata. Signed-off-by: Jinjun Gao <gjinjun@gmail.com>
commandgjj
force-pushed
the
Enable_SetField_Metadata_Register
branch
from
63c14da to
350e03f
Compare
wenyingd
left a comment
wenyingd
left a comment
There was a problem hiding this comment.
LGTM. Just one kind remind, set_metadata action is implemented in OFAction, that means we doesn't plan to support it in FlowAction, right?
Yes, we don't use FlowAction currently. |
|
@ashish-varma Would you give a review on this change? Will it introduce conflict if we switch to OF1.5? |
@ashish-varma Can you please take a look this change? Thanks! |
2 participants
WriteMetadata is write-action instruction which will be applied later
than apply-action. In some case, we need use resubmit to do pipeline in
the same table and set Metadata before resubmit at the same time.
WriteMetadata cannot do the job. We need enable set_field for Metadata
register.
Actually, WriteMetadata do the same thing with set_field Metadata
register in the OvS. From OpenFlow 1.5, Metadata register also can be
used with set_field to set, not stricted only using WriteMetadata instruction.
Signed-off-by: Jinjun Gao gjinjun@gmail.com