Nir Soffer has posted comments on this change. Change subject: PoC: enforcement of contract for rpc ......................................................................
Patch Set 2: Code-Review-1 (14 comments) https://gerrit.ovirt.org/#/c/50032/2//COMMIT_MSG Commit Message: Line 7: PoC: enforcement of contract for rpc Line 8: Line 9: This patch demonstrates how we could obtain client version by using Line 10: client-version stomp header for CONNECT frame. This would enable vdsm Line 11: to modify content of response for specific version of the client. I don't like this direction. We should not modify responses based on client version, as this will make debugging much harder. Line 12: Line 13: The idea is that Bridge is responsible for enforcing contract by fixing Line 14: requests parameters based on yaml schema by making sure that api changes Line 15: like additions, removals or modifications do not break backward Line 18: Line 19: Here we explore ideas how we could replace our current schema format Line 20: with yaml. I picked fenceNode verb which is very complex. It uses Line 21: custom types and there are changes in the api over time. Proposed schema Line 22: format do not lose any information from the previous schema. This is a good change, remove all the custom parsing code, and use standard format with mature tools for working with it. Please separate converting the schema from custom json+comments format to yamal from the "contract enforcement" feature. Line 23: Line 24: Please note that this patch contains all the code that is required to Line 25: enable vdsm being our of client version and how to pass it around to Line 26: make it useful during compatibility enforcement operations. https://gerrit.ovirt.org/#/c/50032/2/lib/api/parser.py File lib/api/parser.py: Line 19: import yaml Line 20: import os Line 21: Line 22: Line 23: class SchemaParser(object): This class does not parse anything, yamal parse the schema. This looks more like a SchemaCache, which has one method (load), and one property (schema). This is not a very useful class. Also the name of the module is not correct since we don't parse anything. Instead, we should have a schema.py module, which should provide way to load schema from the schema file, and way to get the cached schema. We don't want to force client to import classes from this module and created instances, when they just want to get the cached schema, and we don't want to provide ways to have multiple cached schema (e.g. created multiple instances of such class). Line 24: Line 25: def __init__(self): Line 26: with open(self.find_schema()) as f: Line 27: self._schema = yaml.load(f) Line 25: def __init__(self): Line 26: with open(self.find_schema()) as f: Line 27: self._schema = yaml.load(f) Line 28: Line 29: def find_schema(self, schema_name='vdsmapi', raiseOnError=True): - Remove raiseOnError, we should always raise on errors. - change the name to vdsm-api-scheme.yaml or vdsm-api.yaml. We don't have to be compatible with bad names in current code. - This function has nothing to do with parsing the schema, so it should not be a method of the parser, make it a regular function in the module. Line 30: """ Line 31: Find the API schema file whether we are running from within the source Line 32: dir or from an installed location Line 33: """ Line 38: installedpath = constants.P_VDSM_RPC Line 39: for directory in localpath, installedpath: Line 40: path = os.path.join(directory, schema_name + '-schema.yaml') Line 41: if os.access(path, os.R_OK): Line 42: return path The os.access check is not needed, if we cannot access the schema something is very wrong with this machine and we should die. Line 43: Line 44: if not raiseOnError: Line 45: return None Line 46: Line 43: Line 44: if not raiseOnError: Line 45: return None Line 46: Line 47: raise Exception("Unable to find API schema file in %s or %s", Define proper exception for this, SchemaNotFound? Line 48: localpath, installedpath) Line 49: Line 50: @property Line 51: def schema(self): https://gerrit.ovirt.org/#/c/50032/2/lib/api/vdsmapi-schema.yaml File lib/api/vdsmapi-schema.yaml: Line 18: # Line 19: Line 20: types: Line 21: # Specifies the type of fencing operation to perform Line 22: FenceNodeAction: &FenceNodeAction > This is how yaml enables references of nodes. Is this needed so you can reference later FenceNodeAction? Line 23: type: enum Line 24: values: ['status', 'on', 'off', 'reboot'] Line 25: Line 26: # A mapping of hostId indexed by domain UUID Line 73: Host.fenceNode: Line 74: params: Line 75: # The IP address of the remote fence agent Line 76: - name: addr Line 77: type: string Is this a really a string or an ipv4/6 address? do we support dns names? Line 78: Line 79: # The port number of the remote fence agent Line 80: - name: port Line 81: type: integer Line 77: type: string Line 78: Line 79: # The port number of the remote fence agent Line 80: - name: port Line 81: type: integer We should have minimum and maximum values for integers, so the bridge can reject invalid requests without duplicating such validation code in the business logic. Line 82: Line 83: # The type of agent being connected to Line 84: # (For example: rsa, ilo, drac5, ipmilan, etc) Line 85: - name: agent Line 82: Line 83: # The type of agent being connected to Line 84: # (For example: rsa, ilo, drac5, ipmilan, etc) Line 85: - name: agent Line 86: type: string Should we use enum here? Line 87: Line 88: # The username used to login to the remote fence agent Line 89: - name: username Line 90: type: string Line 86: type: string Line 87: Line 88: # The username used to login to the remote fence agent Line 89: - name: username Line 90: type: string Any limits on the character set? is this unicode? ascii only? Line 91: Line 92: # The password for username Line 93: - name: password Line 94: type: string Line 94: type: string Line 95: Line 96: # The type of fencing operation to perform Line 97: - name: action Line 98: type: *FenceNodeAction Any limits on the character set? is this unicode? ascii only? Line 99: Line 100: # Enable SSL communication Line 101: - name: secure Line 102: type: boolean Line 99: Line 100: # Enable SSL communication Line 101: - name: secure Line 102: type: boolean Line 103: defaultValue: false Use lowercase for all keys Line 104: Line 105: # Additional agent-specific parameters in space-separated Line 106: # <var>=<val> pairs Line 107: - name: options Line 111: # Additional options needed in fenceNode logic Line 112: - name: policy Line 113: type: *FencingPolicy Line 114: defaultValue: null Line 115: added: 3.5 3.5 is not a good version number, we don't want use floating point numbers for version numbers. We should have either tuples (major, minor) or integer value that is easy to compare, such as 305 (major * 100 + minor). Or a string "3.5" that can be compared may converting it to tuple. Line 116: -- To view, visit https://gerrit.ovirt.org/50032 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia25c58fb7cb90fc74eceb902aa4748b0025cf96f Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
